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

Refactor MessageArgs #3358

Merged
merged 17 commits into from
Jun 27, 2024

Conversation

jeremiah-corrado
Copy link
Contributor

@jeremiah-corrado jeremiah-corrado commented Jun 25, 2024

This PR makes a few changes to improve the ergonomics of argument parsing:

  • added this(key) accessor to MessageArgs, allowing things like const x = msgArgs["x"]
  • modified getGenericTypedArrayEntry to accept ParamObj directly, allowing things like: getGenericTypedArrayEntry(msgArgs["name"], st)
  • added some more generic methods for parsing scalars, and json lists
    • a json list can be easily parsed as a tuple (if size is known at compile time), a Chapel array, or a Chapel list
  • removed unused/uneeded methods from MessageArgs
  • removed objType field from MessageArgsas it was only used in Efunc2 and was easily confused with the ObjType defined in serverConfig (resolves Merge ObjType and ObjectType enums #2522)

Most of the old MessageArgs API still exists, so message handlers can be converted to use the new API over time. As an example, ManipulationMsg was converted to use the new API.

Also cleaned up IO compat modules (most of those changes were moved to #3365), leaving only symbols related to the ioendian->endianness change.

jeremiah-corrado and others added 12 commits June 18, 2024 13:14
…e. Fix misleading indentation in serverDaemon. Remove repeated error-handling code from ServerDaemon.

Signed-off-by: Jeremiah Corrado <[email protected]>
Signed-off-by: Jeremiah Corrado <[email protected]>
…clude 'toScalar', 'toScalarTuple', 'toScalarList', 'toScalarArray'. Other methods were removed or "deprecated". Added 'this()' indexing to msgArgs

Signed-off-by: Jeremiah Corrado <[email protected]>
…rs-R-Us#2522. Only 'efunc2Msg' was using this functionality, so put a temporary fix in place until it can be refactored

Signed-off-by: Jeremiah Corrado <[email protected]>
src/ArgSortMsg.chpl Outdated Show resolved Hide resolved
@jeremiah-corrado
Copy link
Contributor Author

CI failed to install dependencies. Could someone please restart when you get a chance.

@bmcdonald3
Copy link
Contributor

Started CI again

Copy link
Contributor

@ajpotts ajpotts left a comment

Choose a reason for hiding this comment

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

Thanks for doing all this!

Copy link
Contributor

@jaketrookman jaketrookman left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link
Contributor

@bmcdonald3 bmcdonald3 left a comment

Choose a reason for hiding this comment

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

Love it <3

src/Message.chpl Outdated Show resolved Hide resolved
Signed-off-by: Jeremiah Corrado <[email protected]>
Copy link
Member

@stress-tess stress-tess left a comment

Choose a reason for hiding this comment

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

looks good!! Thanks jeremiah!

@stress-tess stress-tess added this pull request to the merge queue Jun 27, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 27, 2024
@jeremiah-corrado
Copy link
Contributor Author

Thanks for the reveiws everyone! It looks like CI failed again due to dependency installation timeouts. Could someone please re-add to the merge queue :)

@stress-tess stress-tess added this pull request to the merge queue Jun 27, 2024
Merged via the queue into Bears-R-Us:master with commit b8c3e9a Jun 27, 2024
16 checks passed
@jeremiah-corrado jeremiah-corrado deleted the refactor-message-args branch June 27, 2024 20:55
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.

Merge ObjType and ObjectType enums
5 participants