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

chore: Tox configuration scripts #718

Closed
wants to merge 16 commits into from
Closed

Conversation

tinvaan
Copy link
Contributor

@tinvaan tinvaan commented Mar 3, 2024

What kind of change does this PR introduce?

Project maintenance, dev workflow enhancement.

What is the current behavior?

#696

What is the new behavior?

Move away from Makefiles to tox configuration scripts. tox is tailor made for python projects compared to Makefiles and allows for venv management and test automation + standardization in CI workflows.

For eg, the below config will run our tests against multiple python environments, to help guard against any breaking changes we introduce and thus ensuring backward compatibility.

[tox]
isolated_build = True
envlist = format, py37, py38, py39, py310, py311, py312

Additional context

Tests against all supported versions can be launched using a single tox command and optionally running tests against specific versions is also supported using the tox -e argument.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

PR Type: Enhancement

PR Summary: This pull request introduces a significant enhancement to the project's development workflow by transitioning from Makefiles to tox configuration scripts. The change aims to leverage tox's capabilities for virtual environment management and test automation, facilitating a more standardized and efficient CI workflow. The PR includes modifications to the project's documentation, deletion of previous workflow scripts, and the addition of new scripts tailored for tox, covering installation, linting, and test coverage.

Decision: Comment

📝 Type: 'Enhancement' - not supported yet.
  • Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
  • Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
  • Files deleted: Sourcery does not currently approve diffs with deleted files.

General suggestions:

  • Ensure that the new email format in the 'Signed-off-by' lines in the CHANGELOG.md is intentional and adheres to standard conventions.
  • Consider adding unit tests for the newly introduced automation scripts, particularly focusing on the Command.run method to ensure its reliability across various scenarios.
  • It would be beneficial to include tests that specifically cover the new tox configuration scripts, ensuring they work as expected across the different Python environments specified.
  • Documenting the expected test coverage threshold within the project's contribution guidelines or directly within the scripts could help maintain or improve test coverage as the project evolves.

Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨

Share Sourcery

Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.



def coverage():
return Command.run("poetry run pytest --cov=./ --cov-report=html -vv")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (llm): While the coverage command is a good addition, it's crucial to ensure that the test suite includes tests that cover the new tox configuration scripts. This includes testing for different Python environments as specified in the tox.ini file. If such tests are not present, I recommend adding them to verify that the tox configurations work as expected across the specified Python versions.

@@ -0,0 +1,19 @@
import subprocess
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (llm): It's great to see automation scripts being added to enhance the development workflow. However, it would be beneficial to include unit tests for these scripts themselves, especially for the Command.run method, to ensure its reliability and handle potential exceptions gracefully. Testing the scripts will help in maintaining the robustness of the development workflow.



def coverage():
return Command.run("poetry run pytest --cov=./ --cov-report=html -vv")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (llm): The addition of a coverage command is commendable as it emphasizes the importance of test coverage. However, it would be beneficial to document the expected coverage threshold within the project's contribution guidelines or as a comment within this script. This ensures that contributors are aware of the coverage expectations and maintain or improve the test coverage with their contributions.

@tinvaan
Copy link
Contributor Author

tinvaan commented Mar 3, 2024

Sorry for the noise, CI finally looks green ✅

@mansueli mansueli changed the title Tox configuration scripts chore: Tox configuration scripts Mar 4, 2024
tox.ini Outdated Show resolved Hide resolved
@tinvaan tinvaan requested a review from mansueli March 5, 2024 03:13
@tinvaan
Copy link
Contributor Author

tinvaan commented Mar 8, 2024

@mansueli , @J0 -- any further thoughts on this?

@J0
Copy link
Contributor

J0 commented Mar 10, 2024

Hey @tinvaan,

I'll have a deeper look soon and get back

Makefile Show resolved Hide resolved
@tinvaan tinvaan requested a review from a team as a code owner March 27, 2024 11:36
@tinvaan tinvaan requested a review from J0 March 27, 2024 11:37
@tinvaan
Copy link
Contributor Author

tinvaan commented Mar 29, 2024

Resolved merge conflicts & painted CI green again.

/cc @J0

@J0
Copy link
Contributor

J0 commented Apr 14, 2024

@tinvaan,

Thanks for the PR! I think the maintaining team will need to look through this before we can decide whether to move forward as it affects the development flow. It may take a while.

cc: @silentworks @mansueli

@silentworks
Copy link
Contributor

Closing this PR for the reasons given on #696 (comment).

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.

4 participants