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

Regenerate hooks list documentations #1902

Open
wants to merge 1 commit into
base: 9.x
Choose a base branch
from

Conversation

tleon
Copy link
Contributor

@tleon tleon commented Dec 19, 2024

Questions Answers
Branch? 8.x
Description? Regenerate hooks list documentations
Fixed ticket? Fixes #37163

@tleon tleon self-assigned this Dec 19, 2024
@github-actions github-actions bot added the 8.x label Dec 19, 2024
@nicosomb
Copy link
Contributor

Why on 8.0.x branch?

@jolelievre jolelievre closed this Dec 19, 2024
@jolelievre jolelievre reopened this Dec 19, 2024
@jolelievre jolelievre changed the base branch from 8.x to 9.x December 19, 2024 22:09
@jolelievre jolelievre changed the base branch from 9.x to 8.x December 19, 2024 22:10
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

I think the command has room for improvements:

  • it is removing some valid documentation like action<ClassName><Action>Before.md, these hooks were detected but skipped in the command so I guess that's why they are removed here
  • we didn't have infos extracted in the command bu here we have some data in the md files, I think the generation command should be able to also parse the data from the documentation from the md with regexp This way if we are not able to generate the description or usages they can be manually added in the doc Then on next generation the command can extract these data and it would avoid erasing some valid content

I block the PR for now until we improve the command but it's a really good start it just need some updates once the command has been improved

Also as @nicosomb said is the target here 8.X on purpose? does it mean you ran this command from the 8.1.x branch? it seems unlikely since the command has only been add on 9.0.x so far so maybe it's the wonrg base branch

Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

I'm blocking it because there are too many issues at the moment.

One thing you probably weren't aware of:
You need to implement a feature in the generator to let us ignore some of the hooks; some from the docs have additional information, and some examples have been added that cannot be generated.

@Hlavtox
Copy link
Contributor

Hlavtox commented Dec 24, 2024

Same opinion as others - there is many useful information in the docs that have been contributed, it would get lost.

@tleon tleon force-pushed the Issue-37163-regenerate-hooks-md-files branch from 90adc60 to 2a6c167 Compare January 2, 2025 16:26
@tleon tleon changed the base branch from 8.x to 9.x January 2, 2025 16:27
@tleon
Copy link
Contributor Author

tleon commented Jan 2, 2025

So I targeted the right branch and the right files.

Regarding the data loss :

I ran the doc generation command directly on the current version of the md folder ( previously I deleted all md in there and regenerated all of them. I was not aware that some of the md may have been modified by hand.)

I'm afraid that even while running the command like this, the files already there have been erased as before.

What I could do is : modify the command to ignore the files already present in the folder ( matching on the name of the files ). This would preserve the md modified by hand but those ones would miss some informations that I added in the command. ( I think ;) )

Also Like it was suggested I can ignore the hook containing a placeholder in the name I did it in the generation of the xml file I can just do the same thing here.

How does that sound ?

@kpodemski @Hlavtox @nicosomb @jolelievre

@tleon tleon requested review from jolelievre and kpodemski January 7, 2025 08:19
@jolelievre
Copy link
Contributor

Hi @tleon

ok good to know that the removal was manual and not caused by the command. I think we can think later about how we could remove automatically the obsolete hooks but let's not get carried away for now.

Regarding the override of existing hooks your suggestion is interesting but I'm thinking we could go one step further, the idea would be that if the file is already present the command only updates the fields that are missing or empty. Any already existing data/text in the MD files would be kept by default so only new data would be used for override.

To implement this we could add a HookDocExtractor service that can read and parse the data for each hook based on each MD file name. Then when you create the data to be exported the priority would be for each field something like Doc > XML fixture > Auto generated data > Default fallback

On the side note while you're on this topic some people on slack noticed that some hook doc pages are empty like this one https://devdocs.prestashop-project.org/9/modules/concepts/hooks/list-of-hooks/displaypdftemplate/ which matches the displayPDF<Template> hook, one theory is that there is a problem because of the charrets <> but it's worth looking into it, it may not be the (only) reason because this one is fine https://devdocs.prestashop-project.org/9/modules/concepts/hooks/list-of-hooks/actionclassnameactionbefore/

@kpodemski
Copy link
Contributor

To implement this we could add a HookDocExtractor service that can read and parse the data for each hook based on each MD file name. Then when you create the data to be exported the priority would be for each field something like Doc > XML fixture > Auto generated data > Default fallback

This may require some changes in how we format information about a given hook, and it sounds a bit complicated, but if it's feasible, and that it could be trusted, then it would ease work on maintaining the list

@jolelievre
Copy link
Contributor

After discussion with @tleon we decided that for now we will do a simple modification in the command The command checks if the MD file already exists it simply skips it, so the command only adds new hooks

This way we are sure that we don't remove some manually modified files that are likely more up to date

If we want to improve this doc generation it will be done in another PR but we'll have to think about what we clearly want and if there's some real value to do it

@tleon tleon force-pushed the Issue-37163-regenerate-hooks-md-files branch from 2a6c167 to c493f63 Compare January 8, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants