Skip to content

Conversation

arschmitz
Copy link
Member

More style updates to please jscs moved overrides to .jscsrc

@arschmitz
Copy link
Member Author

@scottgonzalez fixed the commit message

jscs: {
all: {
options: {
requireCapitalizedComments: null
config: ".jscsrc"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually needed? If there's a bug with maxErrors we should report it and override maxErrors, but keep letting it use its default config file finding.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you meant to comment on the max errors line above but yeah its not a bug its just the default is 50.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I meant this line. The default config: true should work just fine. In my own limited testing it seemed like jscs wouldn't report any errors unless I set config explicitly. Based on our conversation elsewhere it sounded like there's a bug where jscs wouldn't report any errors at all when there are more errors than maxErrors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the config file yes true should be fine. The bug you mentioned with max errors was already reported to them jscs-dev/node-jscs#1722 it is unrelated to this as its only related to running with fix. I added it here just because the normal max is 50 and i believe we want to output them all.

@jzaefferer
Copy link
Member

Apart from the config property above, this looks good. It even fails when there's an error!

@arschmitz arschmitz closed this Sep 11, 2015
arschmitz added a commit that referenced this pull request Sep 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants