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

fix to "development dependencies should be available as extras" #194

Merged
merged 11 commits into from
Mar 31, 2021

Conversation

H-Shay
Copy link
Contributor

@H-Shay H-Shay commented Mar 30, 2021

Hello-here is my first pull request. Hopefully i hit all the requirements from the 'contributing' doc.
Signed-off-by: Hillery Shay shaysquared@gmail.com

@clokep clokep requested a review from a team March 30, 2021 10:59
Copy link
Contributor

@callahad callahad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello-here is my first pull request.

Yay! 🎉 Thank you so much! Everything looks in order, but I had a few small changes to request

In addition to the comment above about setting versions in setup.py, could you also please remove any mentions of manually pip installing those tools from CONTRIBUTING.md and change the documented install command to pip install -e '.[dev]', which should pull in Sygnal and the dev dependencies?

setup.py Outdated
@@ -52,5 +52,6 @@ def read(fname):
"pywebpush>=1.13.0",
"py-vapid>=1.7.0",
],
extras_require={"dev": ["black", "flake8", "isort", "mypy", "mypy-zope", "tox"]},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like our https://github.com/matrix-org/sygnal/blob/master/tox.ini file specifies some specific versions that it expects for these tools; could you please add those in here? (E.g., "black""black==19.10b0")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I can make these changes sometime tonight. Thanks for the feedback! Will also incorporate the changes from the comment below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay all the requested changes have been made and the checks have passed so I hope this is good!

@clokep
Copy link
Member

clokep commented Mar 30, 2021

We should also update tox.ini to install this extra instead of separately installing the requirements!

@callahad
Copy link
Contributor

We should also update tox.ini to install this extra instead of separately installing the requirements!

Ah, yep! From the Tox docs it looks like we should be able to specify:

extras =
  dev

under the [testenv:check_...] sections, dropping the deps and skip_install stanzas. Then it should Just Work.

@clokep clokep requested a review from callahad March 31, 2021 11:23
Copy link
Contributor

@callahad callahad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested everything and it works as expected locally. Yay!

@callahad callahad merged commit 3c263c4 into matrix-org:master Mar 31, 2021
awesome-michael added a commit to Awesome-Technologies/sygnal that referenced this pull request May 6, 2021
Sygnal 0.9.3 (2021-04-22)
=========================

Features
--------

- Prevent the push key from being rejected for temporary errors and oversized payloads, add TTL logging, and support `events_only` push data flag. ([\matrix-org#212](matrix-org#212))
- WebPush: add support for Urgency and Topic header ([\matrix-org#213](matrix-org#213))

Bugfixes
--------

- Fix a long-standing bug where invalid JSON would be accepted over the HTTP interfaces. ([\matrix-org#216](matrix-org#216))
- Limit the size of requests received from HTTP clients. ([\matrix-org#220](matrix-org#220))

Updates to the Docker image
---------------------------

- Remove manually added GeoTrust Root CA certificate from docker image as Apple is no longer using it. ([\matrix-org#208](matrix-org#208))

Improved Documentation
----------------------

- Make `CONTIBUTING.md` more explicit about how to get tests passing. ([\matrix-org#188](matrix-org#188))
- Update `CONTRIBUTING.md` to specify how to run code style and type checks with Tox, and add formatting to code block samples. ([\matrix-org#193](matrix-org#193))
- Document how to work around pip installation timeout errors. Contributed by Omar Mohamed. ([\matrix-org#215](matrix-org#215))

Internal Changes
----------------

- Update Tox to run in the installed version of Python (instead of specifying Python 3.7) and to consider specific paths and folders while running checks, instead of the whole repository (potentially including unwanted files and folders, e.g. the virtual environment). ([\matrix-org#193](matrix-org#193))
- Make development dependencies available as extras. Contributed by Hillery Shay. ([\matrix-org#194](matrix-org#194))
- Update `setup.py` to specify that a minimum version of Python greater or equal to 3.7 is required. Contributed by Tawanda Moyo. ([\matrix-org#207](matrix-org#207))
- Port CI checks to Github Actions. ([\matrix-org#210](matrix-org#210), [\matrix-org#219](matrix-org#219))
- Upgrade development dependencies. Contributed by Omar Mohamed ([\matrix-org#214](matrix-org#214))
- Set up `coverage.py` to run in tox environment, and add html reports ([\matrix-org#217](matrix-org#217))

Change-Id: I14ae821ff2a1562e91fd87ce25f73baaa0b9430b
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.

3 participants