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.

Sign up for GitHub

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

BUG: allow happi search to match int numerics #261

Merged
merged 3 commits into from
Jun 9, 2022
Merged

BUG: allow happi search to match int numerics #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c3b755d
BUG: ints match float counterparts in search
tangkong Jun 8, 2022
1c31ad8
TST: test_search_int_float
tangkong Jun 8, 2022
b1c4876
DOC: pre-release notes
tangkong Jun 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions docs/source/upcoming_release_notes/261-search_nums.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
261 search_nums
#################

API Changes
-----------
- N/A

Features
--------
- N/A

Bugfixes
--------
- Allow int search values to match their float counterparts

Maintenance
-----------
- N/A

Contributors
------------
- tangkong
12 changes: 10 additions & 2 deletions happi/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,16 @@ def search(
continue

elif is_number(value):
logger.debug('Changed %s to float', value)
# value = str(float(value))
if float(value) == int(float(value)):
# value is an int, allow the float version (optional .0)
logger.debug(f'looking for int value: {value}')
value = f'^{int(float(value))}(\\.0+$)?$'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is good as you have it:

  • Requiring it to match the entire integer value - as marked by ^...$
  • With or without the trailing .0.


# don't translate from glob
client_args[criteria] = value
continue
else:
value = str(float(value))
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't a floating point value be somewhat problematic for regex mode? The above comment and continue gave me pause to think about it a bit:

  • Say we passed in "3.14" we might match "3514" or "3x14" just the same. . would be interpreted as a regex anychar, wouldn't it?
  • Does float-based value searching also work as a full match or a partial match? That is, if 12345.678 is in the database and I search for 5.678 what happens? (lazy question...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are good points.

  • I think you're right, ints will always get an escaped ., but floats will get passed through as is. Is this a problem though? We could argue if someone is searching in regex mode they should be escaping their . if they need to?
  • The float-based value searching seems to not work with partial matches, though I didn't do that intentionally.

Copy link
Contributor

@klauer klauer Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We could argue if someone is searching in regex mode they should be escaping their . if they need to?

If someone is doing a regex search with a thing that looks like a floating point value... is it a floating point value or a regex?

... I think it's a regex and probably shouldn't go through the float(str(float)) conversion? 🤷

Is this a problem though?

Likely not - just trying to dig into the implications of the changes in my usual excessive way 😁

Copy link
Member

@ZLLentz ZLLentz Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is an annoyance with using regex in general to search for floating point numbers rather than a unique problem to happi

Float-based values not partial matching almost seems like a feature rather than a bug...

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point about the processing of regex, Ken

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likely not - just trying to dig into the implications of the changes in my usual excessive way 😁

I don't think it's excessive, it's just thorough 😄

Float-based values not partial matching almost seems like a feature rather than a bug...

I wish I could claim credit for that haha

else:
logger.debug('Value %s interpreted as string', value)

Expand Down
23 changes: 23 additions & 0 deletions happi/tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,29 @@ def test_search_z_range(
assert 'No devices found' in conflict_result.output


def test_search_int_float(runner: CliRunner, happi_cfg: str):
int_result = runner.invoke(happi_cli, ['--path', happi_cfg,
'search', '--names', 'z=3'])
float_result = runner.invoke(happi_cli, ['--path', happi_cfg,
'search', '--names', 'z=3.0'])
assert int_result.output == float_result.output

# # TODO: add this test case once edit works on extraneous info
# edit_result = runner.invoke(
# happi_cli,
# ['-v', '--path', happi_cfg, 'edit', 'tst_base_pim', 'z=3.001'],
# input='y'
# )
# assert edit_result.exit_code == 0

# int_result = runner.invoke(happi_cli, ['--path', happi_cfg,
# 'search', '--names', 'z=3'])
# float_result = runner.invoke(happi_cli, ['--path', happi_cfg,
# 'search', '--names', 'z=3.0'])
# assert int_result.output == ''
# assert float_result.output == ''
Copy link
Member

@ZLLentz ZLLentz Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

👍 for leaving a test for future use
I think another way to do this is to add the test (as an independent function) and mark it as xfail



def test_both_range_and_regex_search(client: happi.client.Client):
# we're only interested in getting this entry (TST_BASE_PIM2)
res = client.search_regex(z='6.0')
Expand Down