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

Tox : removed tests/extract #6362

Closed
wants to merge 2 commits into from

Conversation

shubhank-saxena
Copy link
Contributor

@armenzg shifted flake8 and pytest configuration to tox.ini file. Both of them are tested!

@armenzg armenzg self-requested a review April 29, 2020 20:38
Copy link
Collaborator

@armenzg armenzg left a comment

Choose a reason for hiding this comment

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

Moving the configuration inside of tox.ini will have some unintended problems.
I believe we were talking about using pyproject.toml?

For instance, flake8 does not work well outside of a tox environment. In a sense, this change will make the coupling between tox and Treeherder too strong.
If you follow these set up steps [1] and then run flake8 you will see what will happen.

[1] Set up steps

armenzg@Armens-MacBook-Pro treeherder % python -m venv venv
armenzg@Armens-MacBook-Pro treeherder % source venv/bin/activate
(venv) armenzg@Armens-MacBook-Pro treeherder % pip install -r requirements/dev.txt && pip install -r requirements/common.txt

@armenzg armenzg self-requested a review April 29, 2020 20:47
@armenzg
Copy link
Collaborator

armenzg commented Apr 29, 2020

Instead of trying to delete setup.cfg in one shot focus on handling flake8 well that way the PR will be smaller and less convoluted.

@shubhank-saxena
Copy link
Contributor Author

shubhank-saxena commented Apr 29, 2020

Yes sir. We discussed about pyproject.toml file, but flake8 hasn't yet implemented it's support officially. I did try to check the changes. Will handle flake8 better. Pytest is working as expected!

@armenzg
Copy link
Collaborator

armenzg commented Apr 30, 2020

I don't want us to use tox.ini. If flake8 does not work with pyproject.toml then let's leave it within setup.cfg and file an upstream issue.
If pytest can work with pyproject.toml let's only pursue that.
Thanks!

Copy link
Collaborator

@armenzg armenzg left a comment

Choose a reason for hiding this comment

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

Marking as changes requested so it doesn't show as a pending review.

@shubhank-saxena shubhank-saxena changed the title Removed setup.cfg Tox : removed tests/extract Apr 30, 2020
@shubhank-saxena
Copy link
Contributor Author

@armenzg both flake8 and pytest do not support pyproject.toml officially. I guess we will have to wait for their pending source PRs to get merged
https://gitlab.com/pycqa/flake8/-/issues/428
pytest-dev/pytest#1556

Copy link
Collaborator

@armenzg armenzg left a comment

Choose a reason for hiding this comment

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

If that's the case then we should stick to using setup.cfg. I'm not going to break the behaviour I discussed for no tangible gain besides 1 less file in the repo.

Do you want to open a new PR for the --ignore change?

@armenzg armenzg closed this Apr 30, 2020
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.

2 participants