-
Notifications
You must be signed in to change notification settings - Fork 14
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
[MNT] Improve packaging, managing of dependencies, and managing of ignored warning/errors #88
Conversation
This reverts commit faee59d.
If the rest works, I'll try explicitly setting the version for codecov. |
I tried running it on a different job, just to see if the timeout occurs again. |
@adam2393 Which commands from the |
And I think that line: https://github.com/mscheltienne/mne-icalabel/blob/9d35476815d74fd941545855d65d0f59c87741ad/.circleci/config.yml#L253 |
The ones for running checks, building docs, py testing and "clean". Everything else I don't use |
Done I've also cleaned up the Makefile now and edited the installation page. |
Python 3.9 isn't used, so codecov doesn't upload tho I just realized. |
Ahhh yes, that's a dumb one. I forgot that it tests only on the min/max version. Let's revert to 3.10 and see if the issue persists. |
Kay it works! one time error... |
In a downstream PR, we should try to do some maintenance to bring up test coverage to ~95%. Most likely these will come as we discover bugs w the added code for BIDs/GUI/etc. :p |
Timeouts can happen. Let's reread a bit through these changes both of us, and see if we have anything else we want to change. And then, good to merge. |
.github/workflows/unit_tests.yml
Outdated
with: | ||
path: "mne_icalabel" | ||
- name: Run isort | ||
uses: jamescurtin/isort-action@master |
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.
For these, I'm not 100% knowledgeable, is this the official version? E.g. I found this on the web: https://github.com/isort/isort-action.
Same for flake8
and mypy
?
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.
I'm guessing jamescurtin/isort-action
got renamed/moved to the isort
namespace.. as I can't find anymore the GitHub page of jamescurtin/isort-action
and as he his the main (almost only) contributor to that action ;)
Also: https://pycqa.github.io/isort/docs/configuration/github_action.html
Which does point to the new link, but I'm 99% sure it was pointing to jamescurtin namespace before ;)
I'll check them and update them.
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.
Everything else LGTM.
LGTM, merging if CIs come back green. |
@adam2392 Looks like the doc-deploy image does not exist. |
Looking through the different configuration files for the different automatic checker/formatter/tests.. it's a bit messy and most of the stuff copy/pasted from MNE is probably not required here. Especially, I'm a bit worried by all the different ignore-clause which could mask issues.
So my plan is to try to simplify everything and see how it goes, and while I'm at it let's use
pyproject.toml
to its full potential. After the discussion in #49, I started playing around with the different packaging approaches, let's see if I can get something simpler here ;)