Skip to content
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

Merged
merged 5 commits into from Sep 22, 2021

Conversation

reivilibre
Copy link
Contributor

@reivilibre reivilibre commented Sep 10, 2021

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 guess trial is the way, but should be careful not to mislead).

Signed-off-by: Olivier Wilkinson (reivilibre) <olivier@librepush.net>
@reivilibre reivilibre requested a review from a team September 10, 2021 15:28
@H-Shay
Copy link
Contributor

H-Shay commented Sep 13, 2021

For the testing section, would it be possible to just grab the relevant info from the REAME.md doc and add? https://github.com/matrix-org/sydent#testing
This was how I was running the tests.

Copy link
Contributor

@clokep clokep left a 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
Copy link
Contributor

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:

Suggested change
tox -e py
python -m twisted.trial tests

Copy link
Contributor Author

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.

Copy link
Contributor

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... 🤷

Copy link
Contributor Author

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.

@reivilibre reivilibre merged commit 3d9e2ba into main Sep 22, 2021
@reivilibre reivilibre deleted the rei/contributing_start branch September 22, 2021 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants