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

Add capability to de-register syntax from MOOSE App #30114

Merged
merged 2 commits into from
Mar 20, 2025

Conversation

GiudGiud
Copy link
Contributor

@GiudGiud GiudGiud commented Mar 17, 2025

refs #30113

this one is a tad tricky to test imo. I d have to mess with the TestApp.C which is a little unorthodox?

I ll let the reviewer decide if we want coverage on this or if it's simple enough

@GiudGiud GiudGiud self-assigned this Mar 17, 2025
@GiudGiud GiudGiud marked this pull request as ready for review March 17, 2025 18:30
@GiudGiud GiudGiud requested a review from lindsayad as a code owner March 17, 2025 18:30
@moosebuild
Copy link
Contributor

Job Precheck, step Clang format on 8f7d318 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/30114/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format 3403d29f308c36cd2e06331e17c0ac33aafd6558

@moosebuild
Copy link
Contributor

moosebuild commented Mar 17, 2025

Job Documentation, step Docs: sync website on a892754 wanted to post the following:

View the site here

This comment will be updated on new commits.

@moosebuild
Copy link
Contributor

Job Coverage, step Generate coverage on a66bc55 wanted to post the following:

Framework coverage

3403d2 #30114 a66bc5
Total Total +/- New
Rate 85.29% 85.29% -0.00% 33.33%
Hits 109051 109051 - 1
Misses 18808 18811 +3 2

Diff coverage report

Full coverage report

Modules coverage

Coverage did not change

Full coverage reports

Reports

Warnings

  • framework new line coverage rate 33.33% is less than the suggested 90.0%

This comment will be updated on new commits.

@hugary1995
Copy link
Contributor

hugary1995 commented Mar 18, 2025

I've been wishing for this for a long time. Thanks for adding this capability.

I think it would be nice to add a test case, and I feel like it's not too difficult though. You can register a test object called "DeregisterMe", and de-register it in another translation unit. Then you can create a test case using regular expression to search for the error message (on trying to find DeregisterMe).

@hugary1995
Copy link
Contributor

Oh sorry, I left the comment without reading the file changes. This PR is for de-registering action syntax instead of de-registering objects from the registry...

@GiudGiud
Copy link
Contributor Author

oups sorry for the letdown. We can have a feature request for objects, I agree we could use that too

Copy link
Member

@loganharbour loganharbour left a comment

Choose a reason for hiding this comment

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

Should at the very least have a unit test. There's unit tests for Syntax.

I'm not sure how I feel about this, though. I think just allowing the erasure of anything might be dangerious

@GiudGiud
Copy link
Contributor Author

If we erase a syntax, any input with that syntax should error. So it's fairly safe.

And if they replace the actions for that syntax, that is already allowed and SAM does this

Copy link
Member

@loganharbour loganharbour left a comment

Choose a reason for hiding this comment

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

Failing unit build. Otherwise, good

@GiudGiud
Copy link
Contributor Author

works now

@GiudGiud
Copy link
Contributor Author

Parallel modules failure unrelated

@GiudGiud GiudGiud merged commit 3eb2cae into idaholab:next Mar 20, 2025
49 of 51 checks passed
@GiudGiud GiudGiud deleted the PR_unregister branch March 20, 2025 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants