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

Update to new eradicate api #157

Merged
merged 7 commits into from Oct 9, 2020
Merged

Update to new eradicate api #157

merged 7 commits into from Oct 9, 2020

Conversation

Cielquan
Copy link
Contributor

@Cielquan Cielquan commented Sep 16, 2020

Even so the API of eradicate does change the new implementation is not that different.

  • I updated the usage of eradicate's API
  • I added the new CLI flags: --eradicate-whitelist and --eradicate-whitelist-extend which correspond to the appropriate flags from eradicate.
  • I removed the _Options class because I could not find a reason to keep it. Instead it is now a dict.
  • I removed the in_place option which was present in the _Options class because this setting only effects Eradicate.fix_file() which is the 'parent' function to the here used Eradicate.filter_commented_out_code() and does not get called.
  • I fixed test_incorrect_fixture() which should have failed on current master but does not. I dunno why.
  • I added 2 new tests for the new CLI args.
  • I updated the docs with the new CLI args

EDIT: when eradicate 2.0.0 is released I will update the dependency and finish this PR. Then the CI should also succeed. I tested all thing the CI does locally with success.

@sobolevn
Copy link
Member

sobolevn commented Oct 1, 2020

Related #161

@coveralls
Copy link

coveralls commented Oct 4, 2020

Pull Request Test Coverage Report for Build 499

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 489: 0.0%
Covered Lines: 41
Relevant Lines: 41

💛 - Coveralls

@Cielquan Cielquan marked this pull request as ready for review October 4, 2020 09:23
@Cielquan
Copy link
Contributor Author

Cielquan commented Oct 4, 2020

So I updated the dependency to eradicate v2.

The failed CI is because of safety complaining about pip<19.2 used in the py35 test.

@sobolevn
Copy link
Member

sobolevn commented Oct 4, 2020

Let's drop py35 then.

@Cielquan
Copy link
Contributor Author

Cielquan commented Oct 4, 2020

Let's drop py35 then.

Should I do this in this PR or do u want to do this in another PR?

@sobolevn
Copy link
Member

sobolevn commented Oct 4, 2020

Yes, this PR is fine 👍

@Cielquan
Copy link
Contributor Author

Cielquan commented Oct 4, 2020

Py3.5 is dropped.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Awesome work! 👍

flake8_eradicate.py Outdated Show resolved Hide resolved
tests/test_comments.py Show resolved Hide resolved
@Cielquan
Copy link
Contributor Author

Cielquan commented Oct 9, 2020

So finally got the time to fix the CI issue.

Hope now its OK?

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Yes! Great work!

@sobolevn sobolevn merged commit cbe4ec6 into wemake-services:master Oct 9, 2020
@sobolevn
Copy link
Member

sobolevn commented Oct 9, 2020

Thanks a lot! 👍

@Cielquan Cielquan deleted the update-to-new-eradicate-api branch October 9, 2020 21:43
@Surgo
Copy link

Surgo commented Oct 12, 2020

@sobolevn I'm looking forward to it being released it :)

@sobolevn
Copy link
Member

Will do soon 🙂

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.

None yet

4 participants