-
Notifications
You must be signed in to change notification settings - Fork 0
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 ICON-Liskov integration style-guide #363
Conversation
cscs-ci run default |
launch jenkins spack |
Some nitpicky comments. Could we put that into a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be useful to have a short comment pointing users to the section on icon_liskov
in the README.md
to find out about the usage? https://github.com/C2SM/icon4py/blob/main/tools/README.md#icon_liskov
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, should be there now.
Yes, that makes sense. Thanks. Should I move the README also in the new |
Co-authored-by: Samuel <kellerhalssamuel@gmail.com>
Co-authored-by: Samuel <kellerhalssamuel@gmail.com>
- `!$DSL END STENCIL` as close as possible to the original stencil section, no empty line before, one empty line after. | ||
- `!$DSL START FUSED STENCIL` as close as possible to the original stencil sections, one empty line before, one empty line after. | ||
- `!$DSL END FUSED STENCIL` as close as possible to the original stencil sections, one empty line before, one empty line after. | ||
- `!$DSL INSERT` after `!$DSL START STENCIL`, no empty line before, one empty line after. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does !$DSL INSERT
have to be after !$DSL START STENCIL
? Can't it be anywhere in the f90
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I simplified it.
Co-authored-by: Samuel <kellerhalssamuel@gmail.com>
Co-authored-by: Samuel <kellerhalssamuel@gmail.com>
Co-authored-by: Samuel <kellerhalssamuel@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more comments
I guess to keep it consistent with the other namespace packages I would leave the README.md in the root of the folder and only move the style guide to this new |
Co-authored-by: Samuel <kellerhalssamuel@gmail.com>
…dd_liskov_styleguid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Thanks for the improvements! |
Mandatory Tests Please make sure you run these tests via comment before you merge!
Optional Tests To run benchmarks you can use:
In case your change might affect downstream icon-exclaim, please consider running
For more detailed information please look at CI in the EXCLAIM universe. |
cscs-ci run default |
launch jenkins spack |
This PR adds a style-guide for LISKOV statements in ICON. Co-authored-by: Samuel <kellerhalssamuel@gmail.com>
This PR adds a style-guide for LISKOV statements in ICON.