Skip to content

Do not provide inner CONSTANT, Class, Enum, TypeDef and static method. - #48

Open
KengoTODA wants to merge 1 commit into
teppeis:mainfrom
KengoTODA:follow-official-styleguide
Open

Do not provide inner CONSTANT, Class, Enum, TypeDef and static method.#48
KengoTODA wants to merge 1 commit into
teppeis:mainfrom
KengoTODA:follow-official-styleguide

Conversation

@KengoTODA

Copy link
Copy Markdown

Thanks for publishing great and useful project! I use it in my daily development with great joy.

Today I have one suggestion to fix an existing bug. This fix helps us to compile my code with latest Google Closure Library. To know about detail of this bug, please refer google/closure-library#317.

According to official styleguide, we should not provide inner
CONSTANT, Class, Enum, TypeDef and others. Only top-level Class is enough.

https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone=Providing_Dependencies_With_goog.provide#Providing_Dependencies_With_goog.provide

Squashed commit of the following:

commit 5727eea
Author: Kengo TODA <skypencil@gmail.com>
Date:   Sun Nov 9 15:05:48 2014 +0800

    create Parser instance at timing of `beforeEach`

commit 7d9185a
Author: Kengo TODA <skypencil@gmail.com>
Date:   Sun Nov 9 14:44:44 2014 +0800

    refactor

commit ed7dd24
Author: Kengo TODA <skypencil@gmail.com>
Date:   Sun Nov 9 14:25:00 2014 +0800

    pass all tests

commit c5c5ba4
Author: Kengo TODA <skypencil@gmail.com>
Date:   Sat Nov 8 17:41:22 2014 +0800

    implement expected behavior

commit aa23820
Author: Kengo TODA <skypencil@gmail.com>
Date:   Sat Nov 8 17:24:29 2014 +0800

    add test case to make requirement clear

commit dbf9eca
Author: Kengo TODA <skypencil@gmail.com>
Date:   Sat Nov 8 17:09:08 2014 +0800

    make a new file to handle dependencies
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.06%) when pulling ed0db51 on KengoTODA:follow-official-styleguide into c3429c1 on teppeis:master.

@KengoTODA

Copy link
Copy Markdown
Author

Maybe we need to improve not only goog.provide() but also goog.require(), I will do it in next merge request.

@teppeis

teppeis commented Nov 10, 2014

Copy link
Copy Markdown
Owner

@KengoTODA Thanks, I will look later.

@KengoTODA

Copy link
Copy Markdown
Author

I forgot one thing to tell; Here is the official guideline which explains why we need this patch.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants