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

ASTVerifier #214

Merged
merged 6 commits into from
Oct 22, 2021
Merged

ASTVerifier #214

merged 6 commits into from
Oct 22, 2021

Conversation

rogalski
Copy link
Contributor

@rogalski rogalski commented Oct 1, 2021

Improved performance by caching intermediate data during bisect.

Major changes:

  • ASTVerifier
  • fix up of tests to adjust implementation-aware mocks

Closes #211

src/darker/verification.py Outdated Show resolved Hide resolved
src/darker/verification.py Outdated Show resolved Hide resolved
Copy link
Owner

@akaihola akaihola left a comment

Choose a reason for hiding this comment

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

Great implementation! Just a couple minor nitpicks...

@rogalski
Copy link
Contributor Author

rogalski commented Oct 4, 2021

@akaihola

Docstring fixes are pushed.
Mone missing UT for SyntaxError is comparison is pushed.

Comment about API deprecation vs. just removing code still stands.

About lru_cache - I decided to keep implementation as it is. Seems good enough to me.

We are close to getting that ready. Let's make sure we are on the same page regarding next steps.

@akaihola akaihola added the performance Speed or memory usage improvement label Oct 5, 2021
@akaihola akaihola added this to the 1.3.2 milestone Oct 5, 2021
@akaihola
Copy link
Owner

I'm rebasing this on previous merges and force-pushing the branch into your fork (which is something GitHub seems to allow!).

I'd like to have ASTVerifier.assert_equivalent_to_baseline() and its unit test removed since they haven't existed before either. If you're busy right now, I can do that before merging.

@akaihola
Copy link
Owner

I think this is now in a state ready for merging (provided that the tests pass, we'll see in a minute).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Speed or memory usage improvement
Projects
2 participants