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

Refactor step 2: replace Node traversal with ast.NodeVisitor #93

Closed
wants to merge 67 commits into from
Closed

Refactor step 2: replace Node traversal with ast.NodeVisitor #93

wants to merge 67 commits into from

Conversation

cgahr
Copy link
Collaborator

@cgahr cgahr commented May 1, 2023

depends on #91
see #92 for discussion.

Constantin Gahr added 30 commits April 25, 2023 09:14
exclude test_data/samples/* from black, isort, mypy
1a671bb
remove black, isort, mypy config from tox.ini
dbce624
remove black, isort, mypy config from github ci
056f6e2
remove testing of setup.py in tox.ini
ac2840c
remove setup.py
5b8776c
move config from setup.cfg to pyproject.toml
d50431a
remove mention of setup.py in readme
0c8cf68
update tox.ini for PEP517 compatibility
556e674
Merge branch 'master' into pyproject-toml
f4c9aae
update release ci to not use setup.py
8c4190a
remove setup.py from readme
aa7f40b
Merge branch 'master' into fix-72
e1088e0
introduce Config class, that parses pyproject.toml and iterates over …
b5456a8 
…all valid python files
remove redundant find_python_files code, add find_project_root
f6439b9
use new config to iterate over python files
fa2b99f
remove unnecessary error handling, both are handled by config.files()
193af08
add config for ssort
5080f51
update tox.ini for PEP517 compatibility
3955ad8
Merge branch 'pyproject-toml' into fix-72
ce73311
update tests for _files.py
3d55df1
fix bug find_project_root
3b91f35
use tomli for python < 3.11, tomlib for python >= 3.11
6a659df
add tomli dependency
797424b
remove pathspec mypy block since pathspec is not longer needed
cafa5bb
remove test to check for not existent file, this cannot happen anymore
674ef12
mock find_project_root so that pytest does not search for pyproject.t…
91d8317 
…oml beyond the specified test-folder
ssort only sorts the inteded files, not all files under root
192d9ec
Merge branch 'master' into fix-72
be890a4
Merge branch 'master' into fix-72
f18cee6
Revert "remove unnecessary error handling, both are handled by config…
27fee83 
….files()"

This reverts commit 193af08.
Constantin Gahr added 25 commits May 1, 2023 08:36
add skip_glob configuration key to filter files with glob pattern
6bbf597
add tests for Config.is_invalid
ad3ff91
fix a bug where glob pattern was incorrectly applied if path was not …
8c049d1 
…a file
create current_working_dir to make testing easier, add more tests
ee33e99
remove __pycache__ and .pytest_cache to match blacks defaults
77e6705
ignore __pycache__ and .pytest_cache folders
4440ec6
move sort_key_from_iter to _grahps.py
7eb76c0
move single_dispatch to its own file
014f6d1
move cached_method to _statements.py
e6a372a
move remaining functions to _files.py
27ec33e
move single_dispatch to its own file
d4be66c
move cached_method to _statements.py
a55658b
move remaining functions to _files.py
66e64c1
add new py311 exceptions
c78111a
sort exceptions alphabetically
6a89bed
handle new ExceptionGroups i.e. ast.TryStar
6d546c5
add tests for python 3.11
37cde4b
add version guard for ast.TryStar
0958473
add test for exception groups
f8d7e36
rename ast iter_child function
3cc1ab9
replace get_bindings with ast.NodeVisitor implementation
8137fa6
replace get_method_requirements with ast.NodeVisitor implementation
24789d9
replace get_requirements with ast.NodeVisitor implementation
bd9bb7a
remove redundant modules
ec26adf
extract smart NodeVisitor logic to SmartNodeVisitor
a1ababe
@cgahr cgahr force-pushed the refactor-step-2 branch from f74a38f to a1ababe Compare May 1, 2023 18:29
bwhmather
bwhmather reviewed May 3, 2023
View reviewed changes
Copy link
Owner

@bwhmather bwhmather left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this is a no from me.

ast.NodeVisitor has always inexplicably awful performance. I think @jgberry benchmarked a 20% overall increase in runtime over my original @singledispatch implementation, and the current custom dispatch implementation is another 10% to 15% better than that one.

Dispatching based on name is also just less clean than dispatching based on type.

@jgberry
Copy link
Collaborator

jgberry commented May 3, 2023

Yep, see #42 for the performance related discussion. This was a decision we consciously made. Whether it was the right decision, perhaps that can be revisited. There are tradeoffs on either side.

@cgahr
Copy link
Collaborator Author

cgahr commented May 5, 2023

I see that now.

I did some performance profiling too and found similar results..

@cgahr cgahr closed this May 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwhmather bwhmather bwhmather 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.
3 participants
@cgahr @jgberry @bwhmather