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

Fix: CI: isort GitHub action doesn't report expected issues #116

Conversation

elliot-100
Copy link
Collaborator

@elliot-100 elliot-100 commented May 22, 2024

I did misconfigure the black compatibility for the official isort action originally, but doesn't seem to be any documentation on how to do it correctly, and in any case it doesn't show diffs like it's supposed to (raised isort-action #90). So I've re-implemented using command-line interface.

It should really be configured to take config from pyproject.toml, but that's a possible enhancement; this is restoring expected functionality.

NB isort checks are expected to fail on this PR until existing codebase has corrected isort rule run on it. I think is clearest if I do this in another PR. Will do a dry run in my own fork first...

@elliot-100
Copy link
Collaborator Author

elliot-100 commented May 22, 2024

Hmmm, looks like I've hit a snag - I can't immediately fix the existing codebase, as the isort CI check is giving me more errors than when I run isort locally, even though commands appear to be equivalent.

Looks like local command is ignoring the example .py files which are outside the main package, CI including them; and there are maybe some unexpected subtleties when configuring isort using pyproject.toml. I'll have to come back to this in a day or two...

BTW I've switched to ruff format as a replacement for isort +black on other projects.

@elliot-100 elliot-100 force-pushed the fix-115-ci-isort-github-action-doesnt-seem-to-report-expected-issues branch from f9e7471 to 0c38672 Compare May 23, 2024 10:44
elliot-100 added a commit that referenced this pull request May 23, 2024
Lint existing codebase with isort + black to allow fixed CI in #116 to pass checks
@elliot-100 elliot-100 self-assigned this May 23, 2024
@elliot-100 elliot-100 force-pushed the fix-115-ci-isort-github-action-doesnt-seem-to-report-expected-issues branch from 0c38672 to e86d22e Compare May 23, 2024 11:27
@elliot-100 elliot-100 marked this pull request as ready for review May 23, 2024 11:28
@Olen
Copy link
Owner

Olen commented May 23, 2024

Did it work as expected?

@elliot-100 elliot-100 marked this pull request as draft May 23, 2024 11:41
@elliot-100
Copy link
Collaborator Author

elliot-100 commented May 23, 2024

No, in short. I've fixed files inside spond in #117 and the revised CI here in this PR doesn't flag any issues with them.

However there seem to be subtle differences in behaviour of isort on files in the root, outside tests and spond, when running the same isort command locally vs GitHub actions. It doesn't seem to be as simple as the former ignoring them, which was my naïve assumption as per notes in #117. I spotted this when pre-emptively testing on #107

@elliot-100
Copy link
Collaborator Author

elliot-100 commented May 23, 2024

One option would be to only enforce isort on tests and spond in CI for now and worry about other files (examples + manual 'test') later. Opinion on that?

[PS I do think this may not be feasible to fully fix, as isort seems to be semi-abandoned given long-running issues/questions about behaviour, doc gaps, etc]

@Olen
Copy link
Owner

Olen commented May 23, 2024

I am always very pragmatic. Better get something that works partially, and fix the rest later (if needed) than having nothing at all.

@elliot-100
Copy link
Collaborator Author

OK, I'll do that, probably later today, after tidying up. Thanks

… which appears to have bugs and config documentation gaps
@elliot-100 elliot-100 force-pushed the fix-115-ci-isort-github-action-doesnt-seem-to-report-expected-issues branch from 3f9c098 to 4c49a73 Compare May 23, 2024 15:00
…le behaviour which matches equivalent local command in root directory
@elliot-100 elliot-100 force-pushed the fix-115-ci-isort-github-action-doesnt-seem-to-report-expected-issues branch from 4c49a73 to 50b80cf Compare May 23, 2024 15:01
@elliot-100 elliot-100 marked this pull request as ready for review May 23, 2024 15:03
@elliot-100 elliot-100 merged commit 5a09879 into Olen:main May 23, 2024
5 checks passed
@elliot-100 elliot-100 deleted the fix-115-ci-isort-github-action-doesnt-seem-to-report-expected-issues branch May 23, 2024 15:28
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