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

Remove dependency on the flynt tool #6164

Merged
merged 12 commits into from Jul 12, 2023

Conversation

pavoljuhas
Copy link
Collaborator

@pavoljuhas pavoljuhas commented Jun 24, 2023

  • pylint - enable consider-using-f-string check
  • flynt - check and fix all python sources
  • Test package metadata with numpy rather than flynt

Partially fixes #6171

@CirqBot CirqBot added the size: M 50< lines changed <250 label Jun 27, 2023
Use correct markup for ignoring coverage.
And also remove duplicate import.
@pavoljuhas pavoljuhas marked this pull request as ready for review July 6, 2023 01:13
@tanujkhattar
Copy link
Collaborator

Is flynt not able to convert files for which have added explicit TODOs? It looks like we are adding a LOT of TODOs to the codebase -- what's the plan to get rid of them? Can we experiment with automated tools and change these files as part of this PR itself instead of adding a that many TODOs ?

@pavoljuhas
Copy link
Collaborator Author

Is flynt not able to convert files for which have added explicit TODOs? It looks like we are adding a LOT of TODOs to the codebase -- what's the plan to get rid of them? Can we experiment with automated tools and change these files as part of this PR itself instead of adding a that many TODOs ?

The plan is to address TODO-s in a follow-up PR which will only change the string expressions without changing their values, #6171 (comment). The second PR can be added to .git-blame-ignore-revs without masking away the actual changes in .pylintrc and _compat_test.py.

As for the automated changes, I tested with flynt on one file and it seems to do the job. Still I would like to put these no-op changes to a separate PR.

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

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

@pavoljuhas Okay, I'll approve this PR and let's merge it; but can you make sure we get in the follow up soon so we don't leave the TODOs for a long time?

@tanujkhattar tanujkhattar enabled auto-merge (squash) July 12, 2023 21:52
@pavoljuhas
Copy link
Collaborator Author

@pavoljuhas Okay, I'll approve this PR and let's merge it; but can you make sure we get in the follow up soon so we don't leave the TODOs for a long time?

Sounds good will do.

@pavoljuhas pavoljuhas disabled auto-merge July 12, 2023 22:21
@pavoljuhas pavoljuhas enabled auto-merge (squash) July 12, 2023 22:25
@pavoljuhas pavoljuhas merged commit 8e70f77 into quantumlib:master Jul 12, 2023
30 checks passed
@pavoljuhas pavoljuhas deleted the drop-flynt-dependency branch July 14, 2023 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: M 50< lines changed <250
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace flynt checker with pylint consider-using-f-string
3 participants