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

Rename InvalidArgumentsException -> InstantiationException #371

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

pcrosemurgy
Copy link
Contributor

@pcrosemurgy pcrosemurgy commented Oct 11, 2022

  • Tickets addressed: None
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

Note: this PR's renaming changes the sig. of a mission model interface, therefore mission models must be updated.

This PR renames InvalidArgumentsException -> InstantiationException to minimize confusion between arguments being "invalid" (unconstructable, missing, or extraneous) and our "validation" endpoint responses indicating arguments that are "invalid" (failed an @Validation check).

Verification

All tests passing.

Documentation

Related docs updated.

Future work

None.

@pcrosemurgy pcrosemurgy requested a review from a team as a code owner October 11, 2022 23:28
@pcrosemurgy pcrosemurgy requested a review from camargo October 11, 2022 23:29
@pcrosemurgy pcrosemurgy requested review from mattdailis and removed request for goetzrrGit October 11, 2022 23:29
@pcrosemurgy pcrosemurgy force-pushed the fix/rename-invalid-arguments-exception branch from 692d807 to 80bb5fa Compare October 11, 2022 23:31
@pcrosemurgy pcrosemurgy force-pushed the fix/rename-invalid-arguments-exception branch from 80bb5fa to 98f70e7 Compare October 11, 2022 23:32
@pcrosemurgy pcrosemurgy force-pushed the fix/rename-invalid-arguments-exception branch from 47c8ca4 to ac4f724 Compare October 11, 2022 23:43
@pcrosemurgy pcrosemurgy temporarily deployed to e2e-test October 11, 2022 23:43 Inactive
@Twisol
Copy link
Contributor

Twisol commented Oct 11, 2022

I feel like "erroneous" is still roughly synonymous; I could still see confusion arising here.

How about UnconstructibleDirectiveException? That seems to describe the situation at hand more directly -- these arguments are not merely invalid, but they actually prevent the construction of the desired directive.

@Twisol
Copy link
Contributor

Twisol commented Oct 11, 2022

Or, since the exception occurs directly on the instantiate method, we could call it InstantiationException, which is nice and short.

@pcrosemurgy pcrosemurgy force-pushed the fix/rename-invalid-arguments-exception branch from ac4f724 to 6e382bf Compare October 12, 2022 05:23
@pcrosemurgy pcrosemurgy temporarily deployed to e2e-test October 12, 2022 05:23 Inactive
@pcrosemurgy
Copy link
Contributor Author

InstantiationException is cool with me, thanks for the input :)
Just pushed this change.

@pcrosemurgy pcrosemurgy changed the title Rename InvalidArgumentsException -> ErroneousArgumentsException Rename InvalidArgumentsException -> InstantiationException Oct 12, 2022
Copy link
Contributor

@goetzrrGit goetzrrGit 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 to me.

@pcrosemurgy pcrosemurgy merged commit 07b2862 into develop Oct 12, 2022
@pcrosemurgy pcrosemurgy deleted the fix/rename-invalid-arguments-exception branch October 12, 2022 16:52
camargo added a commit to NASA-AMMOS/aerie-ui that referenced this pull request Oct 12, 2022
camargo added a commit to NASA-AMMOS/aerie-ui that referenced this pull request Oct 12, 2022
JosephVolosin pushed a commit to NASA-AMMOS/aerie-ui that referenced this pull request Aug 20, 2024
JosephVolosin pushed a commit to NASA-AMMOS/aerie-ui that referenced this pull request Oct 21, 2024
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.

4 participants