-
Notifications
You must be signed in to change notification settings - Fork 85
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 a primitive contributing guide and tweak PR template. #393
Conversation
Signed-off-by: Olivier Wilkinson (reivilibre) <[email protected]>
For the testing section, would it be possible to just grab the relevant info from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable overall, I left a couple of comments (and also see @H-Shay's).
CONTRIBUTING.md
Outdated
To make sure everything is working as expected, run the unit tests: | ||
|
||
```bash | ||
tox -e py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think changing this will let you uncomment this section:
tox -e py | |
python -m twisted.trial tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, that sounds good.
Thanks to @H-Shay I noticed that the README includes instructions, but says to use trial tests
.
I seem to have forgotten the -e
flag to pip install -e .[dev]
and notice that:
python -m twisted.trial tests
works- but
trial tests
fails
(I only just realised that I probably forgot -e
and that might be the cause.)
I never really appreciated the difference there, but I guess python -m
treats all subdirs of the current working dir as modules, whereas trial tests
expects the tests
modules to be installed.
I guess I will leave a hint about that, since this tripped me up for a few minutes and I suspect it could be confusing to someone not used to Python's modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is trial tests
failing? Usually this is a path issue (it is finding the wrong trial
binary). Using python -m twisted.trial
should generally be the same... 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first, it failed saying that it can't find tests
.
Then I did pip install .[dev]
and it now finds the tests but can't find the templates.
Only with pip install -e .[dev]
does trial
work.
I think this is because invoking trial
will only use the modules available in your venv, but python -m twisted.trial
seems to have the current directory on Python's path? I'm not ultimately that sure either.
Also includes hints for an easy pitfall that I stumbled upon
As per a discussion with Brendan, I have also tweaked the wording to encourage people to add themselves as 'Contributed by ....', since we generally like to see that but empirically not many people add themselves.
Ameliorates #381 but note that the contributing guide still has a couple of TODO blocks commented out, since this repository doesn't use
tox
and I don't actually have an example of how to run the tests since I haven't worked with it (I would guesstrial
is the way, but should be careful not to mislead).