OCA / maintainer-tools

Odoo Maintainers Tools & conventions for OCA members which evaluate and maintain repositories.
GNU Affero General Public License v3.0
277 stars 460 forks source link

oca-gen-addon-readme: exits with no error if `description.rst` is not present #590

Open aleuffre opened 12 months ago

aleuffre commented 12 months ago

Is your feature request related to a problem? the oca-gen-addon-readme command (and of course its pre-commit hook) exits without error code if DESCRIPTION.rst is not present. I think this can lead to undesirable outcomes, like here in this module: https://github.com/OCA/sale-workflow/pull/2771

where the description.rst was not present, the original committer added an empty README file to make the hooks happy and it was never updated even though there were other readme fragments in the folder.

Relevant method:

https://github.com/OCA/maintainer-tools/blob/master/tools/gen_addon_readme.py#L509-L526

Describe the solution you'd like I think the pre-commit hook should fail with an error if no description.rst file exists. Maybe a command option that's then activated in OCA repos?

cc: @francesco-ooops

francesco-ooops commented 12 months ago

@aleuffre I think we can go directly for .md due to the ongoing transition :)

cc @OCA/oca-consultants

francesco-ooops commented 11 months ago

On second analysis, I think also a check on presence of "USAGE" file in the module should be implemented.

If a module does not add anything to user interface per se, it can easily be stated in the file. If that's not the case, I think we can only benefit from having documentation explaining how to use the module.

It's easy to find many legacy modules describing what the module does in "DESCRIPTION", but not how to setup/employ their features, eg: https://github.com/OCA/partner-contact/tree/14.0/base_partner_sequence

@sbidoul @yajo what do you think?

sbidoul commented 11 months ago

When the readme generator was introduced, this was done to not override manually created README files and provide a smooth transition.

Today, I think we can now fail pre-commit if the main README (DESCRIPTION, USAGE, and maybe CONTEXT for new branches) fragments are not present.

TumbaoJu commented 11 months ago

I agree with the proposition done by @sbidoul. The Read Me should have mandatory fragments such as : DESCRIPTION, CONTEXT AND USAGE.

pedrobaeza commented 11 months ago

For me the only mandatory one should be DESCRIPTION.

francesco-ooops commented 11 months ago

For me the only mandatory one should be DESCRIPTION.

how would it hurt if the USAGE file would only contain a standard "This module has no impact on user interface" text?

It's useful for functionals and devs alike, and no cost for developers.

pedrobaeza commented 11 months ago

It's an entry barrier that a module that doesn't have impact on that, requires to put such file for not having an error, and the text that you will find written by developers will not be ideal. Please don't make mandatory something that is not. If you want to add such text as functional, do it with the flows you are improving.

francesco-ooops commented 11 months ago

@pedrobaeza

It's an entry barrier that a module that doesn't have impact on that, requires to put such file for not having an error

I don't see what's the difference between having to add a DESCRIPTION and a USAGE file? if you can add one file, you can add two. All it takes is updating documentation to state both files are required and they will do it, just as with other requirements

and the text that you will find written by developers will not be ideal.

The same developers that can write DESCRIPTION can write USAGE. It doesn't take a special knowledge to write a few steps for USAGE, as nobody is requesting a full-fledged analysis of module use cases or a click-by-click-with-screenshots guide.

What it takes is to help module creator by improving instructions in USAGE template, in order to provide directions regarding what to put there (again, we'll publish a PR for templates asap). Keep in mind that up to this day, there is no mention of how to write documentation in OCA guidelines, and from a functional POV the effects are visible.

Also, I think it's a misconception that developers cannot write good documentation (tons of good USAGE files so far have been written, indeed, by devs). Still, what we're aiming for at the moment is at least decent, not good, documentation. As little as possible as to understand what to do when you open runboat.

If you want to add such text as functional, do it with the flows you are improving.

Yes, functionals hopefully will be always available to improve documentation, but that should not be considered as a cleanup service. Plus, if you're publishing a new module and need someone to do a functional test, you need at least to tell them how to use it.

I think it can only be positive if someone migrating a module can take a bit of time to write down a couple lines on how the final user would use this module.

On the contrary, you're not addressing the fact that legacy modules published when documentation was the last of the problems keep being migrated to this day without USAGE and sometimes even DESCRIPTION... This should fix that. Then of course we can always help improve documentation as soon as needed, but a USAGE file does not look to me like too much to ask.

pedrobaeza commented 11 months ago

As said, for me the only mandatory file should be DESCRIPTION.

francesco-ooops commented 11 months ago

Fair enough, I just wanted to explain my reasoning :)

TumbaoJu commented 11 months ago

Thank you @francesco-ooops for the detailed explanation on why the USAGE fragment should be mandatory.

I totally agree with you and we have faced the same problems with OCA modules over the last years with modules we did not know how to use, so we just decided not to use them.

Sometimes, we could find a developer who would go look into the code and explain to us how to use it but sometimes, even the developer could not tell us how to use the module so, I really think USAGE should be mandatory.

For the CONTEXT fragment, it is new, so we can start to use it and see.

rousseldenis commented 11 months ago

As said, for me the only mandatory file should be DESCRIPTION.

If we want to enhance the user experience with OCA modules, I think USAGE should be mandatory too. Even if its content is short. But it has another use than DESCRIPTION as this one is to describe the WHAT, USAGE is for the HOW.

francesco-ooops commented 11 months ago

Sometimes, we could find a developer who would go look into the code and explain to us how to use it but sometimes, even the developer could not tell us how to use the module so, I really think USAGE should be mandatory.

Very good point, it's a shared experience. It's frustrating to have to ask an internal dev to check the code and provide instructions on how to test an OCA module.

yajo commented 11 months ago

https://thegooddocsproject.dev/ exists to help people have good docs, which start by the readme (template and explanation here). If you want to restructure our readmes or their requirements, I think it's worth investigating.

vdewulf commented 2 months ago

Hello,

We met today with the @OCA/oca-consultants group. We really think that making the "USAGE" mandatory could help improve the documentation of the OCA modules. We want a better discoverability of the OCA modules and a good documentation is a key to that objective. If developers struggle to make a good "usage" description, the consultants working group would be happy to give a hand there.

If we don't want to block a contribution if USAGE is left empty right now, we could still make it mandatory in our usage guidelines that are being written by the working group and will be soon published on the OCA website. This could be a good starting point, in our opinion.

We'll be happy to discuss this matter during the next OCA Days edition in Liège, where a full day will be dedicated to the OCA Documentation Topic.

yajo commented 2 months ago

Let me just point out that there are modules that have no use because are just technical dependencies that need an implementation to actually expose some features.

However, in those cases the contributor can add a simple sentence explaining that, so it shouldn't be a big problem.

francesco-ooops commented 2 months ago

how would it hurt if the USAGE file would only contain a standard "This module has no impact on user interface" text?

It's useful for functionals and devs alike, and no cost for developers.

Exactly what I pointed here @yajo :)

Couldn't that be added to the template itself?