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.

Sign up for GitHub

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

Jump to bottom

Split main file #103

Merged
merged 1 commit into from
Jan 27, 2023
Merged

Split main file #103

merged 1 commit into from
Jan 27, 2023

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented Jan 19, 2023 •
edited
Loading

Kind of a massive overhaul, and somewhat WIP with some file names and structure being stuff I plan to improve on. But this should make working with the files, seeing diffs, interdependence between visitors/tests/files, and all that jazz be easier.

It's rebased on top of #102.

@jakkdl
Copy link
Member Author

jakkdl commented Jan 19, 2023

oops, looks like I broke installation again. Will look at it later today

@jakkdl jakkdl marked this pull request as ready for review January 19, 2023 20:37
@jakkdl jakkdl force-pushed the split_main_file branch 3 times, most recently from 7de702e to 0c80715 Compare January 19, 2023 21:02
@jakkdl
Copy link
Member Author

jakkdl commented Jan 19, 2023

Yay, about time I started figuring out how python packaging works.
Reasonably happy with this structure.

@jakkdl jakkdl mentioned this pull request Jan 19, 2023
Merged
@jakkdl jakkdl force-pushed the split_main_file branch 4 times, most recently from fd6be52 to fafb374 Compare January 26, 2023 10:43
@jakkdl
Copy link
Member Author

jakkdl commented Jan 26, 2023 •
edited
Loading

Ran the code with flake8-type-checking to move some more imports into TYPE_CHECKING blocks since I'd already mostly done that anyway. It also wanted casts to be with string literals, but I ignored that.

Started going through the code to write a review summarizing changes, but since there's barely any code changes it was more like writing file docstrings ... so I just did that instead and enabled flake8-docstrings (but with most docstring requirements ignored). Will do a quick review of the code that's actually meaningful to look at, but otherwise it's just the file structure/organization that's up for review - don't bother looking for differences in the contained code unless I've marked it.

also: note that this is rebased on top of #102, so you can either merge that before looking at this, or just look at the second commit.

@jakkdl jakkdl force-pushed the split_main_file branch 2 times, most recently from b02a9c6 to 331714a Compare January 26, 2023 11:12
jakkdl
jakkdl commented Jan 26, 2023
View reviewed changes
flake8_trio/__init__.py Show resolved Hide resolved
flake8_trio/visitors/__init__.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
tests/test_changelog_and_version.py Show resolved Hide resolved
tests/test_decorator.py Show resolved Hide resolved
tests/test_flake8_trio.py Show resolved Hide resolved
tests/test_flake8_trio.py Show resolved Hide resolved
tox.ini Show resolved Hide resolved
flake8_trio/base.py Show resolved Hide resolved
@jakkdl
Copy link
Member Author

jakkdl commented Jan 26, 2023

Rebased on top of main, and modified flake8_trio/__init__.py/from_filename to be excruciatingly correct 👌

Zac-HD
Zac-HD reviewed Jan 27, 2023
View reviewed changes
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, took me a while to read (and I imagine you a while to write!), but it all looks good to me.

Comments below can be addressed in a follow-up PR rather than this one, to minimize the review/merge-conflict/general overhead burden. And thanks again!

flake8_trio/__init__.py Show resolved Hide resolved
flake8_trio/__init__.py Show resolved Hide resolved
flake8_trio/base.py Show resolved Hide resolved
flake8_trio/visitors/__init__.py Show resolved Hide resolved
tox.ini Show resolved Hide resolved
flake8_trio/visitors/visitors.py Show resolved Hide resolved
flake8_trio/visitors/visitors.py Show resolved Hide resolved
flake8_trio/visitors/visitors.py Show resolved Hide resolved
flake8_trio/visitors/visitors.py Show resolved Hide resolved
flake8_trio/visitors/visitors.py Show resolved Hide resolved
@Zac-HD Zac-HD merged commit 97b4534 into python-trio:main Jan 27, 2023
@jakkdl
Copy link
Member Author

jakkdl commented Jan 27, 2023

No wonder it took you a while to read through it if you took it as an opportunity to re-read the whole codebase! Writing the PR did take me a bit, but that was "just" about moving files, juggling imports and fixing installation - I neither touched nor read the vast majority of the code I was shuffling around.

Will get a PR with various small fixes right away 👍

@jakkdl jakkdl deleted the split_main_file branch January 27, 2023 10:39
@jakkdl jakkdl mentioned this pull request Jan 27, 2023
Closed
12 tasks
@Zac-HD
Copy link
Member

Zac-HD commented Jan 27, 2023

Haha, yeah, I trusted that was the case but GitHub doesn't have a good moved-code view so I just read the lot 😅

@jakkdl jakkdl mentioned this pull request Jan 27, 2023
Merged
@jakkdl jakkdl restored the split_main_file branch January 27, 2023 12:36
@jakkdl jakkdl mentioned this pull request Jan 27, 2023
Closed
5 tasks
@jakkdl jakkdl deleted the split_main_file branch January 28, 2023 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zac-HD Zac-HD Zac-HD left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@jakkdl @Zac-HD