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

Move YAPF checks to separate CI integration #408

Closed
joachimmetz opened this issue Jun 13, 2019 · 3 comments
Closed

Move YAPF checks to separate CI integration #408

joachimmetz opened this issue Jun 13, 2019 · 3 comments

Comments

@joachimmetz
Copy link
Member

The issue with the yapf test is that its results will depend on the version of yapf used. Consider removing the test and have a separate CI yapf test.

@aarontp
Copy link
Member

aarontp commented Jul 3, 2019

@joachimmetz How does moving it to a separate CI test fix the problem of YAPF versions? We could also pin the version of YAPF in the dev dependencies if we really want to do that. I agree it is a little annoying to have tests fail in Travis when they succeed locally, but no matter where we pin the version and what we pin it to, I think this will be an issue.

One somewhat ugly option would be to run it with the last few (2-3?) versions and if any of them pass, then the overall test passes. This would at least reduce the churn/pain.

One reason I like having YAPF as a project test is that it makes breakages more obvious to the developer during development time instead of finding out after creating the PR, seeing the CI errors, and then having to do another commit to fix things.

@joachimmetz
Copy link
Member Author

joachimmetz commented Jul 3, 2019

So the issue is that every dev environment that wants to run the test needs to have the same version of yapf. By moving it to a separate CI test, this limitation goes away. Worst case you as the maintainer can run yapf the right version of yapf on the PR before merge.

E.g. I removed a yapf test from artifacts, since it was causing more grief than benefit

@aarontp aarontp changed the title Consider removing yapf test Move YAPF checks to separate CI integration May 13, 2020
@aarontp
Copy link
Member

aarontp commented Jan 6, 2022

I'm going to close this issue out as this hasn't been a problem in practice since the issue was filed. If it starts being a pain we can re-open.

@aarontp aarontp closed this as completed Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants