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

Support for arrays of strings #332

Merged
merged 3 commits into from
Dec 5, 2024
Merged

Conversation

KaushikMalapati
Copy link
Contributor

Meant to close #327. I made two small changes for this to work

  • In from chain, I moved the data_type.is_string in front of the data_type.is_array or chain.last.array_info so that arrays will not use WaveformRecordPackage.
  • I handle string arrays as a complex type instead of a non-complex type so that a pv is made for each array index

@tangkong tangkong requested a review from ZLLentz December 2, 2024 22:57
@tangkong
Copy link
Contributor

tangkong commented Dec 2, 2024

Conda test failures seem unrelated, we need to add setuptools to the recipe since it's no longer implicit
https://github.com/pcdshub/pytmc/actions/runs/12101972735/job/33797497583?pr=332#step:12:216

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me but I will defer to Zach here.

@KaushikMalapati
Copy link
Contributor Author

Conda test failures seem unrelated, we need to add setuptools to the recipe since it's no longer implicit https://github.com/pcdshub/pytmc/actions/runs/12101972735/job/33797497583?pr=332#step:12:216

Do I just add setuptools to the run requirements in meta.yaml for this?

@tangkong
Copy link
Contributor

tangkong commented Dec 3, 2024

Yes, but I would prefer we make a new PR for this as to be very clear when we modify build recipes. See our other python repos for example conda recipes (we added setuptools_scm gradually as we noticed these errors iirc)
https://github.com/pcdshub/pcdsdevices/blob/6a4e36fbeed085ee3f34ee327a41fcc9e9e7eae4/conda-recipe/meta.yaml#L21

@ZLLentz
Copy link
Member

ZLLentz commented Dec 3, 2024

The changes here make sense in principle but this sort of thing really needs a unit test.

For example, what about adding this new case to the parameterization in

@pytest.mark.parametrize(
"tc_type, is_array, final_type",
[
("BOOL", False, BinaryRecordPackage),
("BOOL", True, WaveformRecordPackage),
("INT", False, IntegerRecordPackage),
("INT", True, WaveformRecordPackage),
("DINT", False, IntegerRecordPackage),
("DINT", True, WaveformRecordPackage),
("ENUM", False, EnumRecordPackage),
("ENUM", True, WaveformRecordPackage),
("REAL", False, FloatRecordPackage),
("REAL", True, WaveformRecordPackage),
("LREAL", False, FloatRecordPackage),
("LREAL", True, WaveformRecordPackage),
("STRING", False, StringRecordPackage),
],
)

(just as one example)

I wonder if there are other places in the test suite that make sense to extend to check the new feature. Perhaps it's worth adding something later in this test_xml_collector near where we check other kinds of "complex" arrays?

I'm not exactly sure what this looks like in practice but the lack of testing is the thing that stands out most here.

@ZLLentz
Copy link
Member

ZLLentz commented Dec 5, 2024

I like the tests you added a lot!

I'm going to click "update branch" and if it passes we should merge this!

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

This is a good expansion of our IOC gen capabilities.

@ZLLentz ZLLentz merged commit 626f312 into pcdshub:master Dec 5, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for arrays of strings
3 participants