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 sort_reexports code mangling #2283

Merged
merged 4 commits into from
Jan 9, 2025
Merged

Conversation

Helveg
Copy link
Contributor

@Helveg Helveg commented Jul 27, 2024

Fixes #2193 and fixes #2252,

The logic behind sort_reexports was keeping track of a point in the output_stream, and seeked to that point before writing the sorted reexports back in, in order to compensate for the first line of this code sorting section being written into the stream already.

The original code tried to track the position of the point with every iteration, but since the code is very branched, and contains ~10 .write statements, this fails, and this approach would never be easily maintained. So I opted to simplify the approach by instead rolling back the write position in the stream by the length of the segment that was written too much.

Additionally, when the input code sort section is longer than the sorted output (e.g., when there is a lot of trimmed whitespace), some previously written garbage remained, which I fixed by truncating the stream after writing to it during sort_reexport operations.

@Helveg
Copy link
Contributor Author

Helveg commented Jul 27, 2024

It seems a bit unfair that DeepSource blocks this PR for cyclomatic complexity while it was already this high :)

@marcinocto
Copy link

👋 This is great, thanks for the fix! Any chance this could be released in any of the next versions?

I agree that it's a bit mad to hold this off on the basis of complexity given how complex the original function is.

@Helveg
Copy link
Contributor Author

Helveg commented Dec 28, 2024

We'd have to find an active maintainer to ping or something, given that there's 50 open pull requests

@Helveg
Copy link
Contributor Author

Helveg commented Dec 28, 2024

@hugovk @timothycrosley sorry for the noise, I'd like to get some traction on this since it's been pending quite some time.

@hugovk
Copy link
Contributor

hugovk commented Dec 28, 2024

@Helveg I don't have merge rights here, perhaps @staticdev can help.

@DanielNoord
Copy link
Member

Thanks! Going to merge this :)

@DanielNoord DanielNoord merged commit 122c09f into PyCQA:main Jan 9, 2025
1 check passed
@DanielNoord DanielNoord mentioned this pull request Jan 9, 2025
@Helveg Helveg deleted the fix-sort-reexports branch January 13, 2025 11: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
4 participants