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.

Sign up for GitHub

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

Jump to bottom

[Linter] Cleanup the linter #50

Merged
merged 11 commits into from
Apr 1, 2014
Merged

[Linter] Cleanup the linter #50

merged 11 commits into from
Apr 1, 2014

Conversation

joshkalpin
Copy link
Member

Part of #48. Changes so far include moving the Result object used by the linter into it's own file and de-duplicating some of the validation running code. I still would like to move all of the attribute validation into it's own class, but this is a start.

@joshkalpin
Move the Linter::Result class into its own file.
6b3de00 
Clears up a bit of the clutter inside of linter.rb and makes it clearly
it's own entity within the linter.
@joshkalpin
De-duplicate validation methods inside of the linter
b24e8d8 
Validation hook runners were essentially the same except by whom they
were targetting.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) when pulling b24e8d8 on linter-refactor into 88f665e on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling fb4d69f on linter-refactor into 88f665e on master.

@fabiopelosin
Copy link
Member

I still would like to move all of the attribute validation into it's own class, but this is a start.

👍 The Linter class is still too big. I would move the Root spec validation helpers and the �All specs validation helpers to two dedicated helper classes. An approach could be to make each method return an array of tuples (type (:error or :warning) and the message.). Maybe we could use a Ruby struct for that.

@joshkalpin
Copy link
Member Author

@irrationalfab I like that idea. This will also let us drastically simplify some of the validation hooks into more concrete steps. For example, the name validator should call _validate_names_match and ensure_no_whitespace or something like that.

@joshkalpin
Copy link
Member Author

Also this should not be merged in until after 0.29.0 is out for obvious stability reasons 😸

@fabiopelosin
Copy link
Member

@kapin 👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling b63f2e0 on linter-refactor into 88f665e on master.

@joshkalpin
Create an analyzer to encapsulate all specs analysis
07bb50e 
tldr; moved a ton of code out of the linter into another class.

TODOS:
- Create a results mixin
- Separate out the validation into two sections for both consumers and
  specs
- Make sure documentation is good
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 07bb50e on linter-refactor into 88f665e on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 8fa24d9 on linter-refactor into 88f665e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-16.42%) to 83.15% when pulling 8fa24d9 on linter-refactor into 88f665e on master.

@fabiopelosin
Copy link
Member

I'm supper happy by the progress on this!

@joshkalpin
Copy link
Member Author

@irrationalfab and @alloy is there any way to get the code climate thing running on this branch? It'll help a wee bit with ironing out the knots.

@orta
Copy link
Member

orta commented Dec 20, 2013

did this get branched?

@joshkalpin
Copy link
Member Author

@orta what do you mean?

@orta
Copy link
Member

orta commented Dec 20, 2013

oh you asked about code climate, my bad

@fabiopelosin
Copy link
Member

@kapin in any case here it is: https://codeclimate.com/github/CocoaPods/Core/compare/linter-refactor

@joshkalpin
Copy link
Member Author

Thanks! This helps a lot :)

@joshkalpin
Merge branch 'master' into linter-refactor
2f85c66 
Conflicts:
	spec/specification/linter_spec.rb
@joshkalpin
Copy link
Member Author

Just an update as I've disappeared off the planet for the past few months. Only about a month of school left in the term and I plan on finishing this up once exams are done. Hopefully, I'll be able to make some serious progress in the week or so I have off before I start work.

@fabiopelosin fabiopelosin changed the title Cleanup the linter [Linter] Cleanup the linter Mar 31, 2014
@fabiopelosin
Copy link
Member

@kapin I was wondering if we can rebase and merge this, and then complete the refactor in a separated pull request when you have some time?

@joshkalpin
Copy link
Member Author

Yes we can. I'll get it ready by the end of the week.

@joshkalpin
Merge branch 'master' into linter-refactor
b597fc1 
Conflicts:
	lib/cocoapods-core/specification/linter.rb
	spec/specification/linter_spec.rb
@joshkalpin
Copy link
Member Author

Just merged the branch in, @irrationalfab if you @alloy and maybe @kylef can take a look at this to make sure I didn't goof on the merge or changed anything I shouldn't have.

@joshkalpin
Remove redundant specs from the linter.
ea328ea 
Porting over the last of the linter specs that are now used by the
analyzer over.
@joshkalpin
Merge branch 'master' into linter-refactor
780dcfc 
Conflicts:
	lib/cocoapods-core/specification/linter.rb
@fabiopelosin
Copy link
Member

Nice thanks! I will review it and merge it!

@fabiopelosin fabiopelosin merged commit 780dcfc into master Apr 1, 2014
@fabiopelosin
Copy link
Member

Looks good merged!

Great work in preventing this to be become outdated

gif

@fabiopelosin fabiopelosin mentioned this pull request Apr 1, 2014
Closed
@alloy
Copy link
Member

alloy commented Apr 2, 2014

Great work, @kapin!

@fabiopelosin fabiopelosin deleted the linter-refactor branch April 2, 2014 10:24
@joshkalpin joshkalpin mentioned this pull request Apr 3, 2014
Closed
5 tasks
@joshkalpin joshkalpin mentioned this pull request May 2, 2014
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@fabiopelosin fabiopelosin

Labels
t1:enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@joshkalpin @coveralls @fabiopelosin @orta @alloy