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

Making a kit from an existing package #93

Merged
merged 11 commits into from
Nov 28, 2023

Conversation

ianmkenney
Copy link
Contributor

Adding documentation for creating an MDAKit from an existing package.

@IAlibay
Copy link
Member

IAlibay commented Nov 4, 2023

@ianmkenney let me know when you want me to review this

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.

I like the idea of coming at the problem of creating a registered MDAKit from a more generic angle and giving hints about licenses etc. I am missing some concrete examples, though. We don't need a tutorial with a fixed case study. But some semi-generic lines of code that we typically see in the registry. Have a look at what we currently have in the meta.yamls and perhaps an obvious pattern emerges.

Ultimately, I think this section should prepare a developer to fill in the metadata.yml file (see https://mdakits.mdanalysis.org/add.html#template-metadata-yaml-file ); look at the comments in the file on the page as they are currently our primary documentation ( :-( ). If you could expand on what's there then that would be really helpful. If you have some examples for what's expected (or worked elsewhere) then that would also show devs what to shoot for.

docs/source/makingakit.rst Outdated Show resolved Hide resolved
docs/source/makingakit.rst Outdated Show resolved Hide resolved
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.

Some change requests (see comments) and please spell-check. Examples are good and tone/level of expertise is appropriate.

docs/source/makingakit.rst Outdated Show resolved Hide resolved
docs/source/makingakit.rst Outdated Show resolved Hide resolved
docs/source/makingakit.rst Outdated Show resolved Hide resolved
docs/source/makingakit.rst Outdated Show resolved Hide resolved
docs/source/makingakit.rst Outdated Show resolved Hide resolved
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.

Thanks @ianmkenney , looking good.

I have some minor suggestions and a broader question, but nothing that would actually block this PR.

docs/source/add.rst Show resolved Hide resolved

Registering an existing package as an MDAKit is a straightforward process.
Since the structure of an MDAKit is not as strict as the code found in the MDAnalysis core library, chances are very little restructuring is needed for registration.
The primary concern is ensuring that the core MDAKit :ref:`requirements<requirements>` are met, as listed at the top of this document.
Copy link
Member

Choose a reason for hiding this comment

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

This ref works; just IIRC the sphinx docs say somewhere "space between text and link name", i.e., :ref: requirements <requirements>. Come to think, if title and link name are the same, then just

:ref:`requirements`

should work. (But test!)

docs/source/makingakit.rst Outdated Show resolved Hide resolved
docs/source/makingakit.rst Outdated Show resolved Hide resolved
@orbeckst orbeckst changed the title WIP: Making a kit from an existing package Making a kit from an existing package Nov 18, 2023
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 for this @ianmkenney, looks great!

I've left a couple of comments for some changes, with one of them being a blocker (@orbeckst's suggestion to switch to git clone latest).

docs/source/add.rst Show resolved Hide resolved
docs/source/makingakit.rst Outdated Show resolved Hide resolved
docs/source/makingakit.rst Show resolved Hide resolved
docs/source/makingakit.rst Outdated Show resolved Hide resolved
Although this is the minimum, we highly recommend that you consider generating your documentation with dedicated tools such as `Sphinx <https://www.sphinx-doc.org/en/master/>`_, which allows you to generate static documentation using `reStructuredText <https://www.sphinx-doc.org/en/master/usage/restructuredtext/index.html>`_ formatted plain-text directly from your code.
This makes it easier for your documentation to change alongside code changes.

Using a documentation hosting service such as `Read the Docs <https://readthedocs.org>`_ or `GitHub Pages <https://pages.github.com/>`_ makes public access to your generated documentation automatic.
Copy link
Member

Choose a reason for hiding this comment

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

@fiona-naughton these sections might be good entry points for further guides, i.e. this can eventually be switched to "see our guide on building docs for mdakits here".

docs/source/makingakit.rst Outdated Show resolved Hide resolved
docs/source/makingakit.rst Outdated Show resolved Hide resolved
docs/source/makingakit.rst Outdated Show resolved Hide resolved
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 @ianmkenney, sorry about the delayed review!

@IAlibay IAlibay merged commit ab1a327 into MDAnalysis:main Nov 28, 2023
2 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.

3 participants