Skip to content

gh-105540: Improve relative paths in generate_cases.py #106942

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

Closed
wants to merge 1 commit into from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Jul 21, 2023

In the original code before c413207#diff-65feee563fa44b472b07751184c6f557699f84b3c2ef79174bfd5d538d748272 there were three places where we had os.path.relpath calls. I've changed them to be just prettify_filename calls.

Now I am adding os.path.relpath back to all these three places by including it in prettify_filename.

Plus, changing a comment that is a bit misleading.

@gvanrossum
Copy link
Member

I'm sorry, I don't think I want to do this. It would mean that if the script is run from a directory other than the repo root, errors in the input will be reported incorrectly, and that may confuse users (or even editors).

We only need the relative path in the generated output files, which is exactly where they were before this PR (and after mine).

@sobolevn
Copy link
Member Author

sobolevn commented Jul 21, 2023

We only need the relative path in the generated output files, which is exactly where they were before this PR (and after mine).

Oh, ok 👍

I was restoring the original behavior from pre-#106713 where we had 3 places where os.path.relpath was used:

  1. filename = os.path.relpath(self.stream.name, ROOT)
  2. filename = os.path.relpath(filename, ROOT)
  3. os.path.relpath(filename, ROOT).replace(os.path.sep, posixpath.sep)

Please, feel free to close :)

@gvanrossum
Copy link
Member

I think we accidentally stumbled upon a better way. :-)

@gvanrossum gvanrossum closed this Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants