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

Address linting and test errors #5

Merged
merged 7 commits into from
Oct 3, 2024
Merged

Conversation

tsalo
Copy link
Contributor

@tsalo tsalo commented Oct 3, 2024

Apologies for the unsolicited PR. I mostly just wanted to see why tests were failing in the last commit, but the logs were expired.

@paquiteau
Copy link
Owner

Hi, no worries, Any help is welcome!

@tsalo
Copy link
Contributor Author

tsalo commented Oct 3, 2024

It looks like the linter-check job runs black, but style issues flagged by that step don't affect the job's status? I.e., it's passing now, but if I navigate to the Black Check step in the CI log there are a bunch of things black would change.

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Oct 3, 2024

@tsalo: aren't you afraid that adding the word typo in the title of the PR may summon the codespell demon? (AKA @yarikoptic) 😜

@tsalo
Copy link
Contributor Author

tsalo commented Oct 3, 2024

oohhh good point! 😄 Plus this is mostly about getting the linter and tests passing at this point anyway

@tsalo tsalo changed the title Fix miscellaneous typos Address linting and test errors Oct 3, 2024
@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Oct 3, 2024

yay: all green !

@paquiteau paquiteau self-requested a review October 3, 2024 14:06
@paquiteau paquiteau merged commit b34360b into paquiteau:master Oct 3, 2024
3 checks passed
@paquiteau
Copy link
Owner

Thank you for the clean up! I guess we have a good starting point to built upon :)

@tsalo tsalo deleted the fix-typos branch October 3, 2024 14:22
@tsalo
Copy link
Contributor Author

tsalo commented Oct 3, 2024

Weird... CI is failing on master. https://github.com/paquiteau/patch-denoising/actions/runs/11163580095/job/31030882890

Also the docs build needs to be fixed.

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Oct 3, 2024

is that a flaky test ?

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Oct 3, 2024

I can reliably get it locally.

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Oct 3, 2024

weird: the fail happens on #7 on python 3.8 but not on #6 that does not run 3.8 but I get the fail locally with python 3.12...

@Remi-Gau
Copy link
Collaborator

Remi-Gau commented Oct 3, 2024

OK my bad: when running locally in proper virtual environment (and not my usual conda 'ecosystem' - larger and meaner than an environment), everything is OK.

So I would try merging #6 to stop supporting py3.8 and get rid of the error.

@tsalo
Copy link
Contributor Author

tsalo commented Oct 3, 2024

Sounds good to me!

@yarikoptic
Copy link
Contributor

@tsalo: aren't you afraid that adding the word typo in the title of the PR may summon the codespell demon? (AKA @yarikoptic) 😜

image

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