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 more Python 2 code leftovers (tool 2to3) #1084

Merged
merged 1 commit into from
May 2, 2023

Conversation

hartwork
Copy link
Contributor

Follow-up to #1074

CC @bwendling

Copy link
Member

@bwendling bwendling left a comment

Choose a reason for hiding this comment

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

After this, do we need the yapflib/py3compat.py file anymore?

@hartwork
Copy link
Contributor Author

hartwork commented May 2, 2023

After this, do we need the yapflib/py3compat.py file anymore?

@bwendling it still defines three symbols:

  • PY38
  • raw_input
  • removeBOM

We can move things elsewhere, and then delete the file if you like
The first two symbols seem test-related, the third is used by the core at a single place.

To summarize the two key questions:

  • Should the file be resolved by moving the related code?
  • If yes, should it be done in here or in a follow-up pull request?

PS: Resolving conflicts in a second…

@hartwork hartwork force-pushed the remove-python-2-leftovers branch from c835c2f to 46b1c0a Compare May 2, 2023 19:48
@bwendling
Copy link
Member

Okay, so I suggest moving PY38 and raw_input to the yapftest/ subdirectory and the placing the removeBOM into the file using it. That should be done as a separate PR.

Thank you!

@bwendling bwendling merged commit a22db83 into google:main May 2, 2023
@hartwork
Copy link
Contributor Author

hartwork commented May 2, 2023

@bwendling sounds good, PR upcoming…

@hartwork hartwork mentioned this pull request May 2, 2023
@hartwork hartwork deleted the remove-python-2-leftovers branch May 2, 2023 21:08
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