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

Escape regex special characters in FITS keyword values used for IFC filtering #770

Merged
merged 10 commits into from May 20, 2021
4 changes: 4 additions & 0 deletions CHANGES.rst
Expand Up @@ -10,6 +10,10 @@ Other Changes and Additions
Bug Fixes
^^^^^^^^^

- When filtering an ``ImageFileCollection`` by keyword value, and not
explicitly using a regex search pattern, escape parentheses in the keyword
value for a successful search. [#770]

2.1.1 (2021-03-15)
------------------

Expand Down
2 changes: 2 additions & 0 deletions ccdproc/image_collection.py
Expand Up @@ -710,6 +710,8 @@ def _find_keywords_by_values(self, **kwd):
pattern = re.compile(value,
flags=re.IGNORECASE)
else:
# Add escape characters to parentheses:
value = value.replace('(',r'\(').replace(')',r'\)')
Copy link
Member

Choose a reason for hiding this comment

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

I think this would fix #769 it were this instead, so that all special characters were escaped:

Suggested change
value = value.replace('(',r'\(').replace(')',r'\)')
value = re.escape(value)

# This pattern matches the prior behavior.
pattern = re.compile('^' + value + '$',
flags=re.IGNORECASE)
Expand Down
5 changes: 5 additions & 0 deletions ccdproc/tests/test_image_collection.py
Expand Up @@ -1100,3 +1100,8 @@ def test_filtered_collection_with_no_files(self, triage_setup):
ifc = ImageFileCollection(triage_setup.test_dir)

ifc_no_files = ifc.filter(object='really fake object')

def test_filter_with_parenthetical_keyword_values(self, triage_setup):
# Would need test data containing FITS keywords whose values have ()
# e.g.: FILTREAR= 'Clear (C)' / Rear Filter
pass
Copy link
Member

Choose a reason for hiding this comment

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

I think the short example that @mkelley provided in #769 could be used as the body of the test here, or at least as the start of a test. It generates a FITS file with a special character in it then tries to filter by a value with the special character.

I can add a credit to @mkelley for the commit with the test body if you end up using a lot of his sample code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I will add this test this week. Change to use re.escape() already committed.

Copy link
Member

Choose a reason for hiding this comment

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

Excellent, thanks!