Add contributing file #307
Conversation
Co-authored-by: Ben Weisburd <[email protected]>
There was a problem hiding this comment.
Thanks for this @tristanlatr. I noticed a couple of typos and make some small suggestions.
CONTRIBUTING.rst
Outdated
|
|
||
| - Create an issue to report a bug or suggest a new feature | ||
| - Triage old issues that need a refresh | ||
| - Implement existing issues |
There was a problem hiding this comment.
suggest: Implement fixes for existing issues
| Pre-commit checks | ||
| ~~~~~~~~~~~~~~~~~ | ||
|
|
||
| - Run unit tests: ``pytest``. |
There was a problem hiding this comment.
suggest: Run unit tests: pytest (or python -m unittest)
Just because pytest may not be installed, and is not necessary to run our tests (though it does provide a nicer interface)
There was a problem hiding this comment.
Let's stick with pytest, it also has cool features like parameterizations etc
There was a problem hiding this comment.
I think you miss my point. The existing tests in this project do not actually use pytest, they use unittest. It's just that the pytest command has a feature of also picking up and running the unittest tests. I'm not denying that pytest has good features, just noting that it's not necessary to install the pytest package to run the existing test suite.
There was a problem hiding this comment.
Its best to stay with a single testing framework that everyone uses, and not mix and matching. Just because python -m unittest works now, doesn't mean that it will work tomorrow.
There was a problem hiding this comment.
Its best to stay with a single testing framework that is part of the Python standard library and currently used for all the existing tests in this module, and not mix and matching. Just because the third-party pytest works now, doesn't mean that it will work tomorrow.
There was a problem hiding this comment.
I think my point I was trying to make is that the project is using pytest today, and thus it will work tomorrow. I'm planning to add (if the powers that be allow me) the xdist plugin to make the tests run faster -- that will most definitely break python -m unittest.
There was a problem hiding this comment.
Your statement is factually incorrect and a non-sequitur. I don't even have the pytest module installed, and all the tests still run. The CI environment does run the pytest command but you can switch it to run python -munittest directly, drop the pytest dependency, and nothing changes. It's the unittest module that is running the tests under the hood, and it always has been.
Similarly I can install nose2 and run all the tests by running nose2. It works, but this doesn't mean that the tests in this module are "using" nose2.
The entire test suite runs on my 10 year old laptop in 3.39 seconds, so I'd personally be sceptical that it needs new dependencies on special plugins to speed it up.
There was a problem hiding this comment.
I see this is a contentious point for you. I am not the maintainer, and I don't have enough skin in the game for it to matter either way. I'll leave this to the maintainer. Best of luck.
CONTRIBUTING.rst
Outdated
| Contributing to ConfigArgParse | ||
| ------------------------------ | ||
|
|
||
| What can you do |
There was a problem hiding this comment.
suggest: What you can do (or else add a ?)
CONTRIBUTING.rst
Outdated
| - Some optional changes requested | ||
| - Some required changes are requested | ||
| - An issue should be opened to tackle another problem discovered during the coding process. | ||
| - If a substential change is pushed after a review, a follow-up review should be done. |
There was a problem hiding this comment.
substential -> substantial
CONTRIBUTING.rst
Outdated
|
|
||
| --- | ||
|
|
||
| owner: people with administrative rights on the repo, only them are able to push a new tag. |
CONTRIBUTING.rst
Outdated
| For example, use ``1234-some-brach-name`` as the name of the branch working to fix issue 1234. | ||
| Once your changes are ready and are passing the automated tests, open a pull request. | ||
|
|
||
| Don’t forget to sync your fork once in while to work from the latest revision. |
There was a problem hiding this comment.
once in while -> once in [a] while
| - At this point, the *owner* should checkout the release branch, | ||
| created and push a new tag named after the version i.e. ``1.5.1``, | ||
| this will trigger the PyPi release process, monitor the process in the GitHub action UI... | ||
| - Update the version and append ``.dev0`` to the current |
There was a problem hiding this comment.
This should partly be handled by setuptools-git-versioning but TBH the more I read about how this is supposed to interact with the release process the more confused I am. Might have to do some mock releases on a test project to get a handle on it.
There was a problem hiding this comment.
We can also stick with a semi-automated process like we do for pydoctor: https://pydoctor.readthedocs.io/en/latest/contrib.html#releasing-and-publishing-a-new-package
| ~~~~~~~~~~~~~~ | ||
|
|
||
| - Code changes and code added should have tests: untested code is buggy code and should | ||
| not be accepted by reviewers. |
There was a problem hiding this comment.
I 100% agree with this, but it took me quite a while in my own programming career to really see the light of unit testing, and writing effective tests for this module can be a bit complex (eg. use of Mock, patch, method replacement, and the undocumented self.initParser()). I wonder if we could give a little more encouragement? Maybe link to a tutorial like this?
There was a problem hiding this comment.
Good point. I think creating a simple example of adding a unit test based on a past issue might make sense. Maybe the -- arg separator is a good example?
There was a problem hiding this comment.
Cool. Rather than hold up this PR just now I'll just make an issue to add this later (added #308)
|
You guys should now have the permissions to merge this. |
|
@bw2 could you enable the “Update branch” button from the settings ? This will enable me to update the branch from the UI instead of doing it from a development computer. |
|
ok Merging this! A Follow-up PR to clarify the process once we have automatic publishing in place would be required. In the mean time, this is good. |
This outlines the processes that I think we all agreed on in #300.
@tbooth @agyoungs @bw2
The release process should probably be adjusted to closely match the target state of the repository tooling