Add test coverage for scripts #375
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Note that I've attempted as unchanged as possible while making sufficient changes to allow for sensible testing.
Note that I've attempted as unchanged as possible while making sufficient changes to allow for sensible testing. In the course of doing this, I realised that there was a flaw in the logic and the `filename` usage message would never be displayed, because an IndexError would be raised when `ARGV.fetch(1)` was called when ARGV is a single element array. I've fixed this and simplified the code in doing so.
Consistency is good and in any case, I think this is simpler and easier to understand.
chrislo
approved these changes
Jan 3, 2018
Yes - I agree - see my comment here: #326 (comment). I've added #380 to capture the idea properly, because we've talked about it quite a few times and I couldn't see it in the list of issues. |
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've extracted the two custom scripts (
bin/create_asset&bin/update_asset) into methods on a new class (CLI) and added a spec for that class.In doing this I identified a slight flaw in the argument parsing of
bin/update_assetwhich I have fixed. Other than that I've attempted to keep the code as unchanged as possible, albeit with some changes to make testing easier. This is not because I think the code is especially good, but because I wanted to minimise the chance of introducing bugs.I did contemplate turning them into Rake tasks as we did in Manuals Publisher, but I realised that would mean updating the corresponding documentation.