"Gemify" #9
"Gemify" #9
Conversation
NickLaMuro
force-pushed
the
gemify
branch
2 times, most recently
from
5f5b4b9 to
0c46fa8
Compare
|
@juliancheal I think I have this where I think it makes sense, so feel free to chuck 🍅 as you see fit. |
There was a problem hiding this comment.
So I was going to put this as "Request Changes"... but apparently I can't do that on my own PR...
Anyway, using this "Review" as a note for a few things that I think might be worth changing here prior to a merge or as a follow up. At a minimum, it might makes sense to at least determine if we handle them here or in a followup.
-
:upstream_branchisn't being properly used in a few places... well, just one, and that is here:code-extractor/code_extractor.rb
Line 38 in 68e6e90
Where
upstream/masterhas been hardcoded where it shouldn't be. I actually changed that in [WIP] Add reinsert functionality #11 (here at the time of writing), but curious if I should fix that sooner rather then that PR, especially since that one probably is doomed to not see the light of day. I would add a test around that me thinks as well.
Bah... my memory is failing me, but I am pretty sure there was at least one other thing that really got merged into PR #11 that probably should exist prior, but just making a mental note of at least one of them that I can think of currently. Probably follow ups are fine, and I will just push the fix to the bin/ after I use that to test with the code I have in #11 (since that is a good real world use case).
Minimal code changes required to make this a gem with an executable script included on install.
- Used minitest since it comes with ruby now, and was easy to work with - Uses `rugged` to help with git validation - There is a large `TestRepo` test helper class that was added The `TestRepo` is probably the elephant in the room, since there is a lot of code being added for it here, but this was intentional to help facilitate adding new features in the future while allowing some coverage for what already exists. As mentioned in the comments, it was lifted from this library in manageiq: https://github.com/NickLaMuro/manageiq/blob/master/spec/support/fake_ansible_repo.rb#L1 Part of this commit: NickLaMuro/manageiq@652105e4 I have added some extra DSL methods an functionality not found in the original, and removed all of the "Ansible bits", so it is a bit of a departure from that code, but the core interface is the same. I think it makes writing tests for this project relatively easy. In addition, there is a `CodeExtractor::TestCase` class in that same `test_helper.rb` file which handles creating a sandbox for each test method. You can also run the tests with `DEBUG=1 rake` to avoid deleting the directories after each run, which is nice for debugging your tests when it isn't doing what you expect.
|
@NickLaMuro sorry I have been keeping an eye on this. Are we good to go do you think @Fryguy? |
|
Yeah this is good. I think any bug fixes or extra things mentioned in #9 (review) should be a follow up PR, since they don't belong in the Gemify PR. |
Converts this project to be distributable on rubygems.
Doesn't change much, and you can still install this as a standalone script, but provides a easier way to support adding features to the project going forward.
Additions:
bin/filerake(included with most rubies)minitest(included with most rubies)rugged(required for running testsMore info in the specific commits.