Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add labels to all the linter warnings and errors #122

Merged
merged 2 commits into from
May 2, 2014

Conversation

joshkalpin
Copy link
Member

Every warning and error should now begin with [CATEGORY]. Updated a
bunch of tests as well to check that these are included.

Still a bit of a WIP, but this should make @Keithbsmiley a bit happier.

@@ -50,7 +50,7 @@ def lint
run_root_validation_hooks
perform_all_specs_analysis
else
error "The specification defined in `#{file}` could not be loaded." \
error "[Spec] The specification defined in `#{file}` could not be loaded." \

Choose a reason for hiding this comment

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

Line is too long. [86/80]

@segiddins
Copy link
Member

Why not then create a new method that takes an error category and message and can then sort the errors by category (and ultimately line number)?

@joshkalpin
Copy link
Member Author

@segiddins that might be a better approach, the line number isn't the responsibility of the linter backend, but I can see how this could work. There needs to be a refactor of the results class (on my todo list) as well, but I figured this would be a quick way to make things a tad easier.

@joshkalpin
Copy link
Member Author

Also it appears @houndci hates me too...

@segiddins
Copy link
Member

@kapin gotcha. Haven't yet spent too much time with the linter

@keith
Copy link
Member

keith commented Apr 28, 2014

This is definitely headed in the direction I think is good. One thing. Can the text inside the brackets directly specify the attribute? For example. Sources would be source and weak frameworks would be weak_frameworks

@joshkalpin
Copy link
Member Author

@Keithbsmiley definitely can do that.

@keith
Copy link
Member

keith commented Apr 28, 2014

Also github sources. And whatever else you need to do for line numbers. 😃

@joshkalpin
Copy link
Member Author

@Keithbsmiley, let me give some though on the line numbers, it may require more of a dig into spec DSL to parse the line number when we read in the podspec.

@keith
Copy link
Member

keith commented Apr 28, 2014

If it would be parsing and not tracking as they were loaded I think it would make sense to expose that from something like a --show-linenumbers

@@ -50,7 +50,7 @@ def lint
run_root_validation_hooks
perform_all_specs_analysis
else
error "The specification defined in `#{file}` could not be loaded." \
error "[spec] The specification defined in `#{file}` could not be loaded." \

Choose a reason for hiding this comment

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

Line is too long. [86/80]

@fabiopelosin
Copy link
Member

Ace work!

@@ -50,7 +50,8 @@ def lint
run_root_validation_hooks
perform_all_specs_analysis
else
error "The specification defined in `#{file}` could not be loaded." \
error "[spec] The specification defined in `#{file}` could not be" \
" loaded." \

Choose a reason for hiding this comment

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

Prefer single-quoted strings when you don't need string interpolation or special symbols.

Every warning and error should now begin with [CATEGORY]. Updated a
bunch of tests as well to check that these are included.
@fabiopelosin
Copy link
Member

Awesome work... this can be merged after the changelog entry is added!

@fabiopelosin
Copy link
Member

FYI to fix rubocopo/houndci issues you can run $ rubocop --autofix

@joshkalpin
Copy link
Member Author

Added so merging 😺

joshkalpin pushed a commit that referenced this pull request May 2, 2014
Add labels to all the linter warnings and errors
@joshkalpin joshkalpin merged commit 88e41eb into master May 2, 2014
@joshkalpin joshkalpin deleted the better_linter_msgs branch May 2, 2014 15:40
@keith
Copy link
Member

keith commented May 2, 2014

🌠

@alloy
Copy link
Member

alloy commented May 2, 2014

Damn nice, @kapin!

Ashton-W pushed a commit to Ashton-W/Core that referenced this pull request Nov 2, 2015
Add labels to all the linter warnings and errors
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.

6 participants