FAIRmat-NFDI / cookiecutter-nomad-plugin

MIT License
1 stars 1 forks source link

Feedback on autogenerating new plugins #26

Closed JosePizarro3 closed 1 month ago

JosePizarro3 commented 3 months ago

Here you have some feedback

JosePizarro3 commented 3 months ago
JosePizarro3 commented 3 months ago
blueraft commented 3 months ago

It is unclear that plugin_name and modulename are inserting at the beginning a string nomad in the names.

I can remove nomad_ from user input, which should resolve this.

I'll suggest a more elaborated logic, or simply reading the real location of the repo (if this is possible...); the github_username should only write the authors/maintainers fields.

The logic might be too complicated here. You can generate the template files locally without a repo too. Perhaps rewording the question to github username/organisation might be sufficient here?

The description of an entry point class in the init.py could perhaps be copied from the description of the plugin passed? It seems to me too generic to be useful, but I understand this might be not the point. For the future, it will be nice that the name of the classes of the parser/schema plugins can be passed. I don't know if this is possible, but it will save some much time.

I'll create separate issues for these.

Also, a better explanation of the auto-generated README is needed. We did something for nomad-simulations, but maybe you can check: https://github.com/nomad-coe/nomad-simulations/blob/develop/README.md

Which parts specifically? I think Adding this plugin to NOMAD should live in nomad documentation and not plugin documentation, so maybe just a link to that page on nomad documentation suffice here.

Please, delete the lines populating Results in the parser. A parser is never going to have this responsability, and I am very much against this.

Might have to wait for Lauri for this, I don't really have the background here.

Why there is a MANIFEST.in?

This is for backwards compatibility if you also have old-style plugins in the repo that you haven't migrated yet.

We can have a chat about the isort thing in private.

JosePizarro3 commented 3 months ago

The logic might be too complicated here. You can generate the template files locally without a repo too. Perhaps rewording the question to github username/organisation might be sufficient here?

What about separating, i.e., having github_username and organization, and if organization is left empty, it fallsback to the username?

Which parts specifically? I think Adding this plugin to NOMAD should live in nomad documentation and not plugin documentation, so maybe just a link to that page on nomad documentation suffice here.

Please, read nomad-simulations, and compare with an auto-generated template. The readme is pretty cryptic in the former, and this is usually the first thing that users read.

Might have to wait for Lauri for this, I don't really have the background here.

We don't think we need to wait, @lauri-codes has pretty much the same opinion than us. A very important message here is that external users must not have access to the results section. This is a section exclusively under control of FAIRmat, and as such, we need to have it under control.

Otherwise, Interoperability is broken.

Also, I doubt the layers of normalization in the src code of NOMAD are making those lines useless. Now, about writing into data: yes, users can do whatever they want.

blueraft commented 3 months ago

The readme is pretty cryptic in the former, and this is usually the first thing that users read.

I did read the nomad-simulation readme, maybe you can create a PR updating the readme and we can discuss the changes then? https://github.com/FAIRmat-NFDI/cookiecutter-nomad-plugin/blob/main/nomad-%7B%7Bcookiecutter.plugin_name%7D%7D/README.md

We don't think we need to wait, @lauri-codes has pretty much the same opinion than us.

Well it's code he wrote 😬

Feel free to make a PR modifying this file: https://github.com/FAIRmat-NFDI/cookiecutter-nomad-plugin/blob/main/nomad-%7B%7Bcookiecutter.plugin_name%7D%7D/py_sources/src/parsers/myparser.py

JosePizarro3 commented 3 months ago

Cool, that sounds good, I'll do it!

JosePizarro3 commented 1 month ago

@blueraft because of the upcoming Hackathon, I will do all these minor changes at once:

lauri-codes commented 1 month ago

Yes please go ahead and remove the results thing. The only reason I used is because I wanted to have the the parser example work without any access to a custom schema (you might instantiate a plugin with only a parser, no schema packages). Maybe the parser example can use some built-in schema instead? I think there are some in the nomad-FAIR package...?

JosePizarro3 commented 1 month ago

I can use basesections.py with a very dummy example of assigning a name (like with the config parameters)

blueraft commented 1 month ago

Feel free to try but Remove the appended nomad_ when generating the repo (https://github.com/FAIRmat-NFDI/cookiecutter-nomad-plugin/issues/28) is not trivial.

The nomad_ suffix is hard coded in a lot of places at the moment (including in the directory names src/{{cookiecutter.module_name}}, or nomad-{{cookiecutter.plugin_name}} in pyproject.toml) so they will all need to be handled dynamically.

There are ways to inject extra variables using cookiecutter, but they didn't reliably work for me with cruft

JosePizarro3 commented 1 month ago

Feel free to try but Remove the appended nomad_ when generating the repo (https://github.com/FAIRmat-NFDI/cookiecutter-nomad-plugin/issues/28) is not trivial.

The nomad_ suffix is hard coded in a lot of places at the moment (including in the directory names src/{{cookiecutter.module_name}}, or nomad-{{cookiecutter.plugin_name}} in pyproject.toml) so they will all need to be handled dynamically.

There are ways to inject extra variables using cookiecutter, but they didn't reliably work for me with cruft

Yes! I will go through it and change it back, same as with my<whatever> naming