Skip to content

Split main file - #103

Merged
Zac-HD merged 1 commit into
python-trio:mainfrom
jakkdl:split_main_file
Jan 27, 2023
Merged

Zac-HD merged 1 commit into
python-trio:mainfrom
jakkdl:split_main_file

Conversation

@jakkdl

@jakkdl jakkdl commented Jan 19, 2023 •

Copy link
Copy Markdown
Member

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

jakkdl commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

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

jakkdl commented Jan 19, 2023

Copy link
Copy Markdown
Member Author

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

@jakkdl
jakkdl force-pushed the split_main_file branch 4 times, most recently from fd6be52 to fafb374 Compare January 26, 2023 10:43
@jakkdl

jakkdl commented Jan 26, 2023 •

Copy link
Copy Markdown
Member Author

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
Comment thread flake8_trio/__init__.py Resolved
Comment thread flake8_trio/visitors/__init__.py Resolved
Comment thread setup.py Resolved
Comment thread setup.py Resolved
Comment thread tests/test_changelog_and_version.py Resolved
Comment thread tests/test_decorator.py Resolved
Comment thread tests/test_flake8_trio.py Resolved
Comment thread tests/test_flake8_trio.py Resolved
Comment thread tox.ini Resolved
Comment thread flake8_trio/base.py Resolved
@jakkdl

jakkdl commented Jan 26, 2023

Copy link
Copy Markdown
Member Author

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

@Zac-HD Zac-HD left a comment

Copy link
Copy Markdown
Member

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!

Comment thread flake8_trio/__init__.py Resolved
Comment thread flake8_trio/__init__.py Resolved
Comment thread flake8_trio/base.py Resolved
Comment thread flake8_trio/visitors/__init__.py Resolved
Comment thread tox.ini Resolved
Comment thread flake8_trio/visitors/visitors.py Resolved
Comment thread flake8_trio/visitors/visitors.py Resolved
Comment thread flake8_trio/visitors/visitors.py Resolved
Comment thread flake8_trio/visitors/visitors.py Resolved
Comment thread flake8_trio/visitors/visitors.py Resolved
@Zac-HD
Zac-HD merged commit 97b4534 into python-trio:main Jan 27, 2023
@jakkdl

jakkdl commented Jan 27, 2023

Copy link
Copy Markdown
Member Author

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
10 of 12 tasks
@Zac-HD

Zac-HD commented Jan 27, 2023

Copy link
Copy Markdown
Member

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 restored the split_main_file branch January 27, 2023 12:36
@jakkdl jakkdl mentioned this pull request Jan 27, 2023
5 tasks done
@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants