add yaml format check in pre-commit #13

unkcpz · 2020-08-10T14:09:43Z

pre-commit check of all yaml files in the repository, with yamlfmt
format yaml file automatically and with yamllint check the final format.

unkcpz · 2020-08-10T14:12:22Z

hi @ltalirz , I think unify the format of yaml files would be a good start of automatic test #9 .
If you agree with this draft, I will go ahead with adding it in github action.

ltalirz

thanks @unkcpz !

I'm a little bit torn here, since I see it as an advantage if people can add yaml files without having to install pre-commit etc. (this means, e.g. it can be done directly via the github web interface).

At the same time I definitely see the benefit of uniformity... i.e. if the linter has clear instructions on how to fix the formatting, let's go ahead.

Please document in the README or wiki how to properly set up the pre-commit hooks.

.yamllint

ltalirz · 2020-08-10T15:37:20Z

computers/daint-mc/daint-mc.yml

+work_dir: /scratch/snx3000/{username}/aiida_run/
+shebang: '#!/bin/bash'
+mpirun_command: srun -n {tot_num_mpiprocs}
+mpiprocs_per_machine: '36'


I guess these quotation marks are introduced because we had quotation marks before, and it wants to keep the information that this should be treated as a string.
However, it would be perfectly fine to treat it as a number as well, i.e. these quotation marks (and those of all other numbers) can be removed.

I guess these quotation marks are introduced because we had quotation marks before

That true. But we can not depend on pre-commit to fix or check this, or we need to customize this rule ourselves (include other customize format errors such as checking invalid input keys).

Ah, don't worry about this for new additions - I just wanted to make sure the examples we have in the repository are as clear as possible (i.e. just change it for the existing ones).
If people go by those examples, it will continue to be like this.

unkcpz · 2020-08-11T01:17:54Z

I was wondering, are there some approaches to automatically fix trivial or non-critical format issues (warnings in yamllint) when user make there PR. The action only failed and then ask user to change the content manually if there are critical issues or issues yamlfmt don't know how to fix (errors in yamllint or errors we defined).
It will maximally reduce the burden of users and in most case they even no need to install the pre-commit.

ltalirz · 2020-08-11T06:32:35Z

was wondering, are there some approaches to automatically fix trivial or non-critical format issues

There are - it could create a new PR with the changes; and we've tried that on aiida-core but it does not work for pull requests from forks, which are the ones where it would matter here.

However, if the output from the linter is good, one can have github actions create a commit comment with the things to change; I think this is the best approach currently.
See e.g the approach here:
https://github.com/aiidateam/aiida-core/blob/1fdcf0f6911ec92f7f79d3f6f88ab6806dbd3dec/.github/workflows/test-install.yml#L235-L273
and how the script forwards information to github actions here
https://github.com/aiidateam/aiida-core/blob/1fdcf0f6911ec92f7f79d3f6f88ab6806dbd3dec/utils/dependency_management.py#L375-L387

unkcpz · 2020-08-11T08:34:33Z

Ha! that's what I am looking for, thanks for the links, I'll have a try.

yakutovicha · 2020-08-11T19:20:34Z

computers/daint-hybrid/[email protected]

+    ###  code prepend_text start ###
+    module load CP2K
+    ###  code prepend_text end ###
+append_text: ' '


I believe this can be an empty string. No need to keep a space here, right? Same for all the cases below.

I think so, don't know why use space here originally. I will change this to empty string.
EDIT: or just remove this parameter in the config file.

That's not true, if for example the append_text is not set here or it is set to empty string '' (without space) . when user call code setup up with verdi, it will prompt and wait user to input. While using ' ' (with space in middle) will fill the parameter. Therefore, it is necessary to leave a space here.

Yes, this is something that will need to be fixed in AiiDA at some point. It is necessary at the moment.

wow, didn't know that. Thanks!

Yes, this is something that will need to be fixed in AiiDA at some point. It is necessary at the moment.

Is there an issue for that?

I checked and didn't find one for this issue specifically. You're welcome to create it

aiidateam/aiida-core#4300

I think the issue has been resolved in the latest release of AiiDA.

yakutovicha · 2020-11-12T21:00:34Z

@unkcpz let us know if you need any help to finalize the PR.

unkcpz · 2020-11-13T10:56:14Z

Thanks @yakutovicha. it's been a bit long time, I will take some time to finish this. I'll let you know if there is any progress. thanks.

ltalirz · 2021-10-14T13:00:33Z

@unkcpz - this is not urgent, but would be nice to get this PR merged at some point.

probably easiest to just take the pre-commit and yamllint configs and rerun them on the registry (to avoid merge conflicts)

unkcpz · 2021-10-14T13:06:30Z

Oh, damn. I totally forget about this. Will finish this before next week.

ltalirz

thanks @unkcpz , looks good to me!
one small question

.pre-commit-config.yaml

The yamllint configuration is set in the file .yamllint. In pre-commit, the strict mode is open to list all the warnings. Therefore all the start yaml document warnings are catched and fixed with adding `---` in front of yaml files.

yakutovicha · 2021-10-21T10:16:28Z

Guys, why did you mess up the repository structure? :(

unkcpz · 2021-10-21T10:22:01Z

@yakutovicha Sorry about that, I didn't notice that. I maybe wrongly rebase the commit. Tell me how to remedy for this, do we need rollback? Or you will make new commits?

yakutovicha · 2021-10-21T10:22:49Z

@yakutovicha Sorry about that, I didn't notice that. I maybe wrongly rebase the commit. Tell me how to remedy for this, do we need rollback? Or you will make new commits?

I just made the PR, please have a look #43.

ltalirz · 2021-10-21T11:18:49Z

Sorry about this - I should have paid closer attention.

I just checked the "Files changed" tab again and it does not show any renamed files, so I would have needed to notice that some files were already named differently than in the repo.

There used to be an option "Require branches to be up to date" in the branch protection rules which I think might have helped here. But I no longer see it https://github.com/aiidateam/aiida-code-registry/settings/branch_protection_rules/22415444

If someone knows where it went or how we could make sure that it would have shown the renamed files here, just let me know.

ltalirz · 2021-10-21T11:21:27Z

P.S. I now see - there are a couple of new files in the "Files changed"; this I should have noticed.

unkcpz marked this pull request as draft August 10, 2020 14:09

ltalirz requested changes Aug 10, 2020

View reviewed changes

unkcpz force-pushed the pre-commit/check-yaml branch from 0353ab0 to 6ab6469 Compare August 11, 2020 01:36

yakutovicha reviewed Aug 11, 2020

View reviewed changes

unkcpz force-pushed the pre-commit/check-yaml branch 2 times, most recently from 3a92c0e to 6ab6469 Compare August 12, 2020 03:13

unkcpz force-pushed the pre-commit/check-yaml branch 6 times, most recently from be6a025 to abb6417 Compare October 19, 2021 08:31

unkcpz marked this pull request as ready for review October 19, 2021 08:32

unkcpz requested review from ltalirz and yakutovicha October 19, 2021 08:32

ltalirz previously approved these changes Oct 19, 2021

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

pre-commit for yaml file check added by yamllint

55a41a7

The yamllint configuration is set in the file .yamllint. In pre-commit, the strict mode is open to list all the warnings. Therefore all the start yaml document warnings are catched and fixed with adding `---` in front of yaml files.

unkcpz dismissed ltalirz’s stale review via 55a41a7 October 19, 2021 09:49

unkcpz force-pushed the pre-commit/check-yaml branch from abb6417 to 55a41a7 Compare October 19, 2021 09:49

ltalirz merged commit b15e903 into aiidateam:master Oct 19, 2021

unkcpz deleted the pre-commit/check-yaml branch October 19, 2021 10:33

yakutovicha added a commit that referenced this pull request Oct 21, 2021

Remove some redundant files introduced in #13

46342d3

yakutovicha added a commit that referenced this pull request Oct 21, 2021

Remove some redundant files introduced in #13 (#43)

5aa1c92

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add yaml format check in pre-commit #13

add yaml format check in pre-commit #13

unkcpz commented Aug 10, 2020

unkcpz commented Aug 10, 2020

ltalirz left a comment

ltalirz Aug 10, 2020

unkcpz Aug 11, 2020

ltalirz Aug 11, 2020

unkcpz commented Aug 11, 2020

ltalirz commented Aug 11, 2020

unkcpz commented Aug 11, 2020

yakutovicha Aug 11, 2020

unkcpz Aug 12, 2020 •

edited

Loading

ltalirz Aug 12, 2020 •

edited

Loading

yakutovicha Aug 12, 2020

yakutovicha Aug 12, 2020

ltalirz Aug 12, 2020

yakutovicha Aug 13, 2020

yakutovicha Nov 12, 2020

yakutovicha commented Nov 12, 2020 •

edited

Loading

unkcpz commented Nov 13, 2020

ltalirz commented Oct 14, 2021

unkcpz commented Oct 14, 2021

ltalirz left a comment

yakutovicha commented Oct 21, 2021

unkcpz commented Oct 21, 2021

yakutovicha commented Oct 21, 2021

ltalirz commented Oct 21, 2021 •

edited

Loading

ltalirz commented Oct 21, 2021

add yaml format check in pre-commit #13

add yaml format check in pre-commit #13

Conversation

unkcpz commented Aug 10, 2020

unkcpz commented Aug 10, 2020

ltalirz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented Aug 11, 2020

ltalirz commented Aug 11, 2020

unkcpz commented Aug 11, 2020

Choose a reason for hiding this comment

unkcpz Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

ltalirz Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yakutovicha commented Nov 12, 2020 • edited Loading

unkcpz commented Nov 13, 2020

ltalirz commented Oct 14, 2021

unkcpz commented Oct 14, 2021

ltalirz left a comment

Choose a reason for hiding this comment

yakutovicha commented Oct 21, 2021

unkcpz commented Oct 21, 2021

yakutovicha commented Oct 21, 2021

ltalirz commented Oct 21, 2021 • edited Loading

ltalirz commented Oct 21, 2021

unkcpz Aug 12, 2020 •

edited

Loading

ltalirz Aug 12, 2020 •

edited

Loading

yakutovicha commented Nov 12, 2020 •

edited

Loading

ltalirz commented Oct 21, 2021 •

edited

Loading