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 the "Maintaining an MDAKit" documentation #223

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

fiona-naughton
Copy link
Contributor

I've added assorted information related to maintaining a kit. This is non-exhaustive so I've kept the "this is under construction" message.

Information that has been added:

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Thanks @fiona-naughton here are some comments.

Copy link
Member

Choose a reason for hiding this comment

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

Could you substitute - with _ here please? For a variety of reasons that'll make things easier to deal with.

- **More tests**: can you get to 100% coverage?!
- **More documentation**: Explain each part of your code. Add examples. Make
your documentation visually appealing and easy to navigate!
- :ref:`Add a logo <logo>`: Spice up your MDAKit with your very own logo!
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a much lower importance thing than the rest, I don't mind it being here, but just pointing out that it's a bit "jarring" in the sense that you have lots of high importance thing and then "logo".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair; I wanted a way to bring it to people's attention that they could have a logo, since it's not mentioned anywhere else (and rule of threes to make this not sound awkward). Perhaps if I added 'you could even...' to make it clearer it comes after the two former things?

- **More documentation**: Explain each part of your code. Add examples. Make
your documentation visually appealing and easy to navigate!
- :ref:`Add a logo <logo>`: Spice up your MDAKit with your very own logo!
- **Use tooling and workflows**: don't rely solely on the MDAKit Registry's CI
Copy link
Member

Choose a reason for hiding this comment

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

Maybe here worth mentioning tools for formatting, etc.. (i.e. your explanation focuses a lot on workflows but not so much on tooling).

- **Release on PyPi/conda-forge**: make it easier for users to install your kit!
- :ref:`Make a journal publication <publishing>`: get recognition for your code!

If applicable, remember to :ref:`update your kit's metadata <update-metadata>`
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to highlight this more to the reader. Would it be worth putting this in a note box?


.. _update-metadata:

Updating the MDAKit's metadata
Copy link
Member

Choose a reason for hiding this comment

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

The fact that we have separate sections for some but not all of the above is a bit "odd" - in the sense that the information flow goes "here's an overview" to "here are some more details on the above". Is there a better way to order this? Separate pages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like splitting things up too much when the sections are this short (makes for a lot of back-and-forth mavigation), but that being said I am planning to add more documentation later that includes details on some of the other above things, and at that stage likely would better to go separate pages.
I'd keep this updating section here since it's generally relevant, but might move the publishing/logo sections


Usually, a package will warn users of any upcoming changes that may affect
downstream usage (e.g. changing how a function is to be used), by raising
a warning indicating the upcoming change when the function is used.
Copy link
Member

Choose a reason for hiding this comment

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

DeprecationWarning (link to Python docs for the warning type)

a warning indicating the upcoming change when the function is used.
If your kit relies on any such to-be-changed features, then (assuming the
relevant code is covered by your kit's tests) these warnings will be triggered
when running the tests and will appear in the logs of the automated CI runs -
Copy link
Member

Choose a reason for hiding this comment

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

... when accessing the deprecated feature. Assuming your unit tests cover this part of your code, such DeprecationWarnings will appear in the output logs..

I would avoid mentioning automated CI probably, i.e. you can see this if you run tests locally or via gh actions.

docs/source/maintaining-a-kit/keep-healthy.rst Outdated Show resolved Hide resolved
@@ -9,19 +9,38 @@ Maintaining an MDAKit
like to see covered here, please get in touch via
`MDAnalysis Github Discussions`_.

Successfully registering an MDAKit is not the end of the journey! Like a pet, a
Copy link
Member

Choose a reason for hiding this comment

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

I see where you're going here - my 2 cents is that the "like a pet" is maybe a bit too much, but I leave it to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's fair - I wasn't actually super happy with this either 😅 but also didn't like how it read jumping straight in - let me think on it again

docs/source/maintainingakit.rst Outdated Show resolved Hide resolved
fiona-naughton and others added 7 commits November 11, 2024 16:03
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
@IAlibay IAlibay self-requested a review November 15, 2024 22:54
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

looks great!

@orbeckst
Copy link
Member

I think all of @IAlibay 's detailed comments have been addressed. I'd be ok with merging to get it out there.

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.

3 participants