-
Notifications
You must be signed in to change notification settings - Fork 107
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
Validate pyproject.toml #557
Conversation
👈 Launch a binder notebook on this branch for commit 3bff82d I will automatically update this comment whenever this PR is modified 👈 Launch a binder notebook on this branch for commit 7d841c7 👈 Launch a binder notebook on this branch for commit a251605 👈 Launch a binder notebook on this branch for commit f29d03a 👈 Launch a binder notebook on this branch for commit 55b3bf8 👈 Launch a binder notebook on this branch for commit 61b1322 👈 Launch a binder notebook on this branch for commit ef7a689 👈 Launch a binder notebook on this branch for commit 1a7c456 👈 Launch a binder notebook on this branch for commit 244c6bc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is maybe my personal preference only, but I don't like what these formatters have done. I feel the project table should be at the top, and I feel that merging the tables together is significantly less readable than previously. I've never seen a pyproject.toml written this way before, and I've seen a lot!
That said, I'm totally in favor of a validator!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 LGTM!
I'd merge, but we need to figure out what's going on with Travis 😱 #558
Let's land #559 so that we can isolate those failures. |
Travis failures are expected and explained by #272 Sorry for the delay, let's roll :) |
Wait... shoot, I can't bypass the check, so I can't merge. I need a new permission to do that :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## development #557 +/- ##
============================================
Coverage 65.07% 65.07%
============================================
Files 36 36
Lines 3052 3052
Branches 538 538
============================================
Hits 1986 1986
Misses 981 981
Partials 85 85 ☔ View full report in Codecov by Sentry. |
The changes from #563 make Travis CI pass but now Codecov is unhappy. |
Looks like I can bypass and merge when CodeCov is failing. Whew! But I won't merge until we have an approver on #563 -- don't want to circumvent any systems in place :) |
.travis.yml
Outdated
script: pytest icepyx/ --verbose --cov app --ignore icepyx/tests/test_behind_NSIDC_API_login.py | ||
script: pytest icepyx/ --verbose --cov app --ignore=icepyx/tests/test_behind_NSIDC_API_login.py --ignore=icepyx/tests/test_auth.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be helpful for future PRs to not include changes that are in another PR (#563)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but… It is quite useful when CI is already failing when the PR is created. PRs that fail are almost never reviewed and merged.
🚀 awesome idea! |
https://github.com/tox-dev/pyproject-fmt
https://github.com/abravalheri/validate-pyproject
Also,
Avoid running tests that require creds on PRs on forks
by adding the changes from #563 to get CI ✅