-
Notifications
You must be signed in to change notification settings - Fork 119
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
Make regex patterns raw strings #605
base: master
Are you sure you want to change the base?
Conversation
Python 3 interprets string literals as unicode strings, so we need to explicitly define regex patterns as raw strings. Silences warnings like 'DeprecationWarning: invalid escape sequence \S'. Search pattern: [\W]re\.[^\.(]+\(['"]
I don't know enough about the team structure in CCTBX to know who to ask for a review. @bkpoon, can I assign this to you for triage? Since the tests pass and there is no new functionality introduced in these changes, I guess it ought to be quite an easy review. |
Looks like I missed a handful of cases because I didn't anticipate white space characters. |
Search pattern used here: \Wre\.\w+\(\s*['"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the tests pass and there is no new functionality introduced in these changes, I guess it ought to be quite an easy review.
The tests that are run on Azure Pipelines is a subset of tests that are run regularly by other developers. The problem is that we have many integration tests that are not run because they take too long or require additional components for the integration test to pass. Moreover, we run Phenix on the entirety of the Protein Data Bank on a regular basis for more extensive testing. So the tests that pass here are necessary, but not sufficient.
It looks like these changes are due to the DeprecationWarning
from the DIALS testing? Something like
2021-04-13T01:18:25.9002977Z ../modules/cctbx_project/iotbx/cif/builders.py:773
2021-04-13T01:18:25.9003505Z /home/vsts/work/1/modules/cctbx_project/iotbx/cif/builders.py:773: DeprecationWarning: invalid escape sequence \S
2021-04-13T01:18:25.9004061Z allmatches = re.findall("(\S*(HL(\S*)[abcdABCD](\S*)))", alllabels )
It'll be easier to approve this pull request if you limit your changes to the code that can be demonstrated to cause the DeprecationWarning
you are encountering. That way, the behavior before and after your changes can be verified.
I have added @Oeffner since he switched to using regular expressions for cctbx_project/iotbx/cif/builders.py
. He'll want to incorporate your changes into his working branch and he is working on updating some tests.
Thanks for finding this! There are some lingering invalid escape sequence
warnings in the Azure Pipelines tests, but I do not think they are related to regular expressions, and I can fix them later.
Python 3 interprets string literals as unicode strings, so we need to explicitly define regex patterns as raw strings.
Silences warnings like
DeprecationWarning: invalid escape sequence \S
.Search pattern used here:
\Wre\.\w+\(['"]
\Wre\.\w+\(\s*['"]