Skip to content

Conversation

@joaomcteixeira
Copy link
Member

@joaomcteixeira joaomcteixeira commented Jul 29, 2022

You are about to submit a new Pull Request. Before continuing, make sure you read the contributing guidelines, and you comply with the following criteria:

  • Your PR is about writing documentation for already existing code 🔥

  • Add a template explaining how to develop a new module.

Please let me know if it is clear or if any information is missing according to your recent experience. Thanks

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022

Codecov Report

Merging #520 (f38793d) into main (164c4f3) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #520   +/-   ##
=======================================
  Coverage   74.66%   74.66%           
=======================================
  Files         105      105           
  Lines        6926     6926           
=======================================
  Hits         5171     5171           
  Misses       1755     1755           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 164c4f3...f38793d. Read the comment docs.

# dependencies required by your module are installed.
# this class method will be executed when HADDOCK3 starts.

# if you module does not import any run-time dependency, just leave
Copy link
Member

Choose a reason for hiding this comment

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

what happens if you put an import here that tries to import a dependency that is not installed, will it handle the error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed, thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the chek @rvhonorato implemented in the optint PR #515 . Maybe we can copy it here? commented out of course

@rvhonorato
Copy link
Member

Thank you very much for this (:

@joaomcteixeira
Copy link
Member Author

thanks for the review. I have addressed all the issues.

@rvhonorato rvhonorato self-requested a review July 29, 2022 08:15
Copy link
Member

@rvhonorato rvhonorato left a comment

Choose a reason for hiding this comment

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

This template should greatly help new developers to add new modules - most likely for the majority of modules nothing will needs to be changed in the core python shell and this template clarifies where things should be added in the module context.

👍🏽

# dependencies required by your module are installed.
# this class method will be executed when HADDOCK3 starts.

# if you module does not import any run-time dependency, just leave
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the chek @rvhonorato implemented in the optint PR #515 . Maybe we can copy it here? commented out of course

@mgiulini
Copy link
Contributor

Good addition indeed, it's going to be useful!

A curiosity: does the check on the choices in the yaml file work?

Co-authored-by: Marco Giulini <54807167+mgiulini@users.noreply.github.com>
@joaomcteixeira
Copy link
Member Author

joaomcteixeira commented Jul 29, 2022

A curiosity: does the check on the choices in the yaml file work?

not yet: #487

I like the chek @rvhonorato implemented in the optint PR #515 . Maybe we can copy it here? commented out of course

what is it?

@sverhoeven
Copy link
Contributor

Works for me, the variables from haddock.modules import modules_category, category_hierarchy dont list the template module and category so my script also does not list them.

@joaomcteixeira
Copy link
Member Author

Works for me, the variables from haddock.modules import modules_category, category_hierarchy dont list the template module and category so my script also does not list them.

Yes, that was my intention. Do you think the template should be listed? I think not because otherwise, users could use it in the workflow, and that makes no sense. Let me know. Cheers.

@sverhoeven
Copy link
Contributor

Works for me, the variables from haddock.modules import modules_category, category_hierarchy dont list the template module and category so my script also does not list them.

Yes, that was my intention. Do you think the template should be listed? I think not because otherwise, users could use it in the workflow, and that makes no sense. Let me know. Cheers.

No it should not be listed, the template is for module contributors, not for module consumers.

@joaomcteixeira
Copy link
Member Author

Perfect 👍 Thanks!

@joaomcteixeira joaomcteixeira merged commit c4883fb into main Aug 5, 2022
@joaomcteixeira joaomcteixeira deleted the module_template branch August 5, 2022 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improve docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants