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 tests for get_changes() method #29

Merged
merged 1 commit into from Oct 29, 2017

Conversation

Projects
None yet
3 participants
@paultcochrane
Contributor

paultcochrane commented Oct 21, 2017

These tests attempt to document the current behaviour of the
get_changes() method.

Interestingly enough, this PR probably creates more questions than it tries to answer. My goal was to increase the code coverage, however in creating the tests I noticed that the implementation of the get_changes() method doesn't really match the documentation for the method in that it doesn't really seem to parse the whole Changes file; it seems to just read the title/header text at the top and return that. Furthermore, the get_changes() method isn't used anywhere in the dist; not even in the release script. It also doesn't read and parse the Changes file for the dist itself, so I'm guessing that something changed in the in past to make the method no longer relevant.

I would guess that you don't want to delete the method since there are possibly clients subclassing it in the manner described in the docs; hence removing it could cause breakage of downstream code, which would be a bad thing. Perhaps it would be best to update method to actually parse the Changes file into a data structure so that the code in release can simply insert the new changes into the relevant position and then write out the new version? One could also accept an optional argument to the method which specifies an alternative Changes file if the user so wishes (this could also make testing a bit easier). What do you think? Is it worth the effort to make such a change?

Add tests for get_changes() method
These tests attempt to document the current behaviour of the
`get_changes()` method.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 21, 2017

Coverage Status

Coverage increased (+1.2%) to 68.172% when pulling c101e65 on paultcochrane:pr/add-tests-for-get-changes into 6532dbf on briandfoy:master.

coveralls commented Oct 21, 2017

Coverage Status

Coverage increased (+1.2%) to 68.172% when pulling c101e65 on paultcochrane:pr/add-tests-for-get-changes into 6532dbf on briandfoy:master.

@briandfoy briandfoy merged commit af4e848 into briandfoy:master Oct 29, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@paultcochrane paultcochrane deleted the paultcochrane:pr/add-tests-for-get-changes branch Nov 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment