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

Expand var compatibility test for active attribute #525

Merged

Conversation

dustinswales
Copy link
Collaborator

@dustinswales dustinswales commented Jan 25, 2024

This PR expands the existing var_compatibility_test to test the functionality of the "active" attribute.

User interface changes? No

Addresses #432

Screenshot of autogenerated code:
Screenshot 2024-01-25 at 3 26 03 PM

test/var_compatibility_test/test_host_mod.meta Outdated Show resolved Hide resolved
test/var_compatibility_test/test_host_data.meta Outdated Show resolved Hide resolved
test/var_compatibility_test/effr_calc.F90 Outdated Show resolved Hide resolved
Copy link
Collaborator

@climbfuji climbfuji 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, thanks for making these changes.

@gold2718
Copy link
Collaborator

gold2718 commented Jan 26, 2024

Please fill in the PR header. I'm not understanding how this addresses #432.

@dustinswales
Copy link
Collaborator Author

@gold2718 Not sure what else to put in there? (There's a code snippet of the correctly autogenerated code)

Copy link
Collaborator

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

Can you tell me why the autogenerated code has the if (.true.) blocks?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not modified in this PR but can you change defdir to be something unique? I like to run all the tests with --cleanup NEVER to compare code and currently, this test overwrites the capgen test.

@dustinswales
Copy link
Collaborator Author

Can you tell me why the autogenerated code has the if (.true.) blocks?

I'm not sure why those lines are in there. Maybe @climbfuji knows their history and/or importance They are unnecessary and make the code tedious to look at.

I just pushed a changed that removes them. For example:
Screenshot 2024-01-31 at 11 07 02 AM

@climbfuji
Copy link
Collaborator

The if (.true.) blocks are there just because it's easier to code - in prebuild, we don't have such a nice way to deal with indentation, therefore it makes like a lot easier to keep it the same regardless of the if condition. But, if you can do away with in in capgen, then that's much better of course!

@dustinswales
Copy link
Collaborator Author

The if (.true.) blocks are there just because it's easier to code - in prebuild, we don't have such a nice way to deal with indentation, therefore it makes like a lot easier to keep it the same regardless of the if condition. But, if you can do away with in in capgen, then that's much better of course!

@climbfuji This makes sense. Thanks for the history. And adios extra lines!
@gold2718 @peverwhee Are we okay to proceed with this PR?

Copy link
Collaborator

@peverwhee peverwhee 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 @dustinswales

@climbfuji
Copy link
Collaborator

@dustinswales Can you confirm that after removing the if (.true.) then blocks, the if statements are still there if the active attribute is not the default .true.?

@dustinswales
Copy link
Collaborator Author

Extension of the auto-generated code snippet from above:
Screenshot 2024-02-05 at 8 36 25 AM

@climbfuji
Copy link
Collaborator

Extension of the auto-generated code snippet from above: Screenshot 2024-02-05 at 8 36 25 AM

Great, thanks!

@dustinswales dustinswales merged commit 6459ff6 into NCAR:feature/capgen Feb 5, 2024
10 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.

4 participants