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

Update CONTRIBUTING.rst #178

Closed
wants to merge 1 commit into from
Closed

Conversation

kobros-tech
Copy link

@kobros-tech kobros-tech commented Aug 9, 2024

remove .hooks as in the documentation it is mentioned to use the hook file in the module's root directory.

It should be "from hooks" instead.

remove .hook as in the documentation it is mentioned to use the hook file in the module's root directory.

It should be "from hook" instead.
Copy link

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

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

Are you sure ?

Done in my V16 env, on my 56 local OCA repos.

rgrep "from hook" ./ --include=*.py | wc -l
0
rgrep "from .hook" ./ --include=*.py | wc -l
124

@kobros-tech
Copy link
Author

kobros-tech commented Aug 9, 2024

@legalsylvain
I have just made a pull request and this error was prohibiting me from contribution.

After the precommit command runs correctly, I must remove hooks.py file and remove the inherit line in init
file, but this is not mentioned in the documentation!

OCA/product-attribute#1706

@legalsylvain
Copy link

After the precommit command runs correctly, I must remove hooks.py file and remove the inherit line in init
file, but this is not mentioned in the documentation!

Which commit remove hook ?

@kobros-tech
Copy link
Author

After the precommit command runs correctly, I must remove hooks.py file and remove the inherit line in init
file, but this is not mentioned in the documentation!

Which commit remove hook ?

@legalsylvain

No commit, I remove hook before commit. I do it manually.

I don't if there any command removes hook, but I had to remove hook as it raised errors while having odoo tests

@legalsylvain
Copy link

Oh. Could you readd the hook you need, and we'll check the error.

thanks.

@kobros-tech
Copy link
Author

@legalsylvain
here it is

Generate addons README files from fragments..............................Failed

  • hook id: oca-gen-addon-readme
  • exit code: 1

@pedrobaeza
Copy link
Member

That error has nothing to do with this text. It just mean that the README generation has failed (or the README is rebuilt). The local import is totally correct for avoiding collapses with global Python libraries. Imagine you have a Python library called hooks...

@kobros-tech
Copy link
Author

@pedrobaeza

I understand now the reason for adding .

but that's what happened, and to make the file be generated successfully with my name as a contributor, I hade to remove the .

I am reporting now and you are the experts to fix errors if they are present.

@pedrobaeza
Copy link
Member

No, you are linking 2 unrelated events. The error is because the README is regenerated. Running the commit/pre-commit command again, you will see that there's no error. Take also into account that for adding you as contributor, you have to modify files inside readme folder, and then, on the next pass of pre-commit, the mentioned hook modifies automatically the README.rst file at module root folder. And this is when it fails. You have to run it again for not having "changes" so it doesn't fail. Please close this PR.

@kobros-tech kobros-tech closed this Aug 9, 2024
@kobros-tech
Copy link
Author

kobros-tech commented Aug 9, 2024

@pedrobaeza
@pedrobaeza

I would have better be notified that running precommit for the first time cause error.

As it is an error, people should be notified.

I spent a lot of time trying to track the issue and asking people in the mailing list.

I must report that if someone is trying to add contribution for the first time the will be lost and give up.

It was not the only error, there was another one about pandoc library that I had to install manually.

I hope the documentation guide us to avoid unnecessary waste of time.

@pedrobaeza
Copy link
Member

The hook is very clear with this:

imagen

so the text you have passes is not the same one as this. The same happens with auto-formatting tools.

If you think that something can be improved in the documentation, please point in which point it's not clear for improving it, but don't argue without even specifying what you have read or not, and leave that tone, or you won't receive more help. I can say as well that you are also "wasting" my time.

@kobros-tech
Copy link
Author

Ok, I already intended to write about my experience to contribute in my blog.

For wasting time issue, I think everybody should help to save their time and others time too.

I made that pull request in order to avoid other new contributors a waste of time.

And instead of making argue, the documentation should refer the that error as it is not critical and should not make the contributor give up.

For me in person I apologise for wasting your time sir, forgive me.

I am sorry.

@pedrobaeza
Copy link
Member

pedrobaeza commented Aug 9, 2024

OK, no problem, just as said, point where do you think we should put that information, as the contribution guidelines don't talk about specific tools. Fow now, I have added a note here:

https://github.com/OCA/maintainer-tools/wiki/Install-pre-commit/_compare/bcf3217b2275b4d17f86bb9f2c7d0af817b2f226...727476d10f946778eabd0197041459492b3a54ce

@kobros-tech
Copy link
Author

It is very good.

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