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

tools: fix regex strings in Python tools #46585

Merged
merged 2 commits into from
Feb 14, 2023
Merged

tools: fix regex strings in Python tools #46585

merged 2 commits into from
Feb 14, 2023

Conversation

josusky
Copy link
Contributor

@josusky josusky commented Feb 9, 2023

tools: fix regex strings in Python tools

Strings used to construct regular expressions shall be marked as raw
otherwise newer versions of Python throw "SyntaxError" because of
invalid escape sequences.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency. labels Feb 9, 2023
@cjihrig
Copy link
Contributor

cjihrig commented Feb 9, 2023

Changes in deps/v8 need to be upstreamed to the V8 project first.

@josusky
Copy link
Contributor Author

josusky commented Feb 9, 2023

OK, so please upstream it, whatever it means ;-)
I am sorry, I am new to nodejs, and I have no idea about the relation to the v8 project. I have just spotted a trivial bug so I have fixed it.

@Trott
Copy link
Member

Trott commented Feb 9, 2023

@nodejs/python

@MoLow
Copy link
Member

MoLow commented Feb 9, 2023

OK, so please upstream it, whatever it means ;-) I am sorry, I am new to nodejs, and I have no idea about the relation to the v8 project. I have just spotted a trivial bug so I have fixed it.

@josusky thanks for this contribution, the meaning is you need to get the change into v8 (following their contribution guide), then if the change is accepted it will automatically be pulled into node

@Trott
Copy link
Member

Trott commented Feb 9, 2023

@josusky thanks for this contribution, the meaning is you need to get the change into v8 (following their contribution guide), then if the change is accepted it will automatically be pulled into node

For clarity: That only applies to the change to deps/v8/tools/gen-postmortem-metadata.py. The changes in the other three files can be submitted in this PR.

@cclauss
Copy link
Contributor

cclauss commented Feb 10, 2023

I do not understand the reason for this pull request. Regex loves raw strings in Python. They are recommended in Python docs.

The solution is to use Python’s raw string notation for regular expressions; backslashes are not handled in any special way in a string literal prefixed with 'r', so r"\n" is a two-character string containing '' and 'n', while "\n" is a one-character string containing a newline. Regular expressions will often be written in Python code using this raw string notation. https://docs.python.org/3/howto/regex.html#the-backslash-plague

Why change from one string to another If two strings are equal to each other?
% python3

>>> r'\s*=.*' == '\s*=.*'
True
>>> r'\\s*=.*' == '\\s*=.*'
False

Please provide example code and a stack trace for where each of these raw strings generates a Python SyntaxError.

Copy link
Contributor

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Please provide example code and a stack trace for where each of these raw strings generates a Python SyntaxError.

@josusky
Copy link
Contributor Author

josusky commented Feb 14, 2023

The point is that in your code you sometimes do not use raw strings. Just look at the code that I have changed. If your python is old enough or not set to report all errors you may not notice the problem, but sooner or later, the deprecated constructions like '\s' will stop working. Just try the following code:

test_string = '\s'

and run it like this python3 -Werror test.py

  File "/home/jano/test.py", line 1
    test_string = '\s'
                  ^^^^
SyntaxError: invalid escape sequence '\s'

@cclauss cclauss merged commit ad0414f into nodejs:main Feb 14, 2023
targos added a commit that referenced this pull request Feb 14, 2023
@targos
Copy link
Member

targos commented Feb 14, 2023

Pull request was landed without a full CI run or metadata in the commit message. I force-pushed main to cancel it. Unfortunately we cannot reopen a merged PR. @josusky can you please open a new one?

@josusky
Copy link
Contributor Author

josusky commented Feb 14, 2023

@targos I am a bit lost. I can certainly create a new PR with the same code change but what should be then the difference to this one?

@targos
Copy link
Member

targos commented Feb 14, 2023

This one was already merged, so we cannot merge it again.

@josusky
Copy link
Contributor Author

josusky commented Feb 14, 2023

Fair enough, the new PR is #46649

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. post-mortem Issues and PRs related to the post-mortem diagnostics of Node.js. python PRs and issues that require attention from people who are familiar with Python. tools Issues and PRs related to the tools directory. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants