aiida-vasp / parsevasp

A general parser for VASP
MIT License
13 stars 13 forks source link

Setup pre-commit #101

Closed atztogo closed 2 years ago

atztogo commented 2 years ago

Most of lines were mechanically modified by tools. I explain about those I did by using my hands below.

As we discussed at issue #31, I set up pre-commit. Tests passed. It was a bit more tough job than I expected, at least. There are still a few failures in pylint, which I would like @espenfl to fix (see below.) Most of complaints were reasonable and I followed them. I wish I didn't break the logic. Some complaints were reasonable, but when I wanted to follow the original code, I used # pylint: disable=.... So if you are interested in those, please grep pylint: #disable.

Three new files: .pre-commit-config.yaml, .style.yapf, pyproject.toml. For settings of these files, I finally referred aiida-quantumespresso. I wanted to respect pydocstyle (now commented out in .pre-commit-config.yaml), but it was postponed. I excluded tests directory from pylint check. I want to include it in the future when I feel my life is easy. Or if somebody else works on it, my life becomes easy.

One important note that I must tell is that I moved the function parsevasp.utils.file_handler to in parsevasp.base.open_close_file_handler (with renamed). Two independent reasons:

  1. Solving circular import
  2. The function name should be different from argument name of itself.

The following is the output of pre-commit run.

% pre-commit run --all-files
Fix double quoted strings................................................Passed
Fix End of Files.........................................................Passed
Fix python encoding pragma...............................................Passed
Mixed line ending........................................................Passed
Trim Trailing Whitespace.................................................Passed
isort....................................................................Passed
flynt....................................................................Passed
yapf.....................................................................Passed
pylint...................................................................Failed
- hook id: pylint
- exit code: 6

************* Module parsevasp.vasprun
parsevasp/vasprun.py:1960:2: W0511: TODO: check in the future if it is faster to fetch all scstep (fixme)
parsevasp/vasprun.py:111:51: E1101: Instance of 'Xml' has no 'ERROR_ONLY_ONE_ARGUMENT' member (no-member)
parsevasp/vasprun.py:112:21: E1101: Instance of 'Xml' has no 'ERROR_ONLY_ONE_ARGUMENT' member (no-member)
parsevasp/vasprun.py:2382:21: E1101: Instance of 'Xml' has no 'ERROR_NO_NBADS' member; maybe 'ERROR_NO_NBANDS'? (no-member)
parsevasp/vasprun.py:3579:51: E1101: Instance of 'Xml' has no 'ERROR_ONLY_ONE_ARGUMENT' member (no-member)
************* Module parsevasp.kpoints
parsevasp/kpoints.py:102:51: E1101: Instance of 'Kpoints' has no 'USE_ONE_ARGUMENT' member (no-member)
parsevasp/kpoints.py:103:21: E1101: Instance of 'Kpoints' has no 'USE_ONE_ARGUMENT' member (no-member)
parsevasp/kpoints.py:109:51: E1101: Instance of 'Kpoints' has no 'USE_ONE_ARGUMENT' member (no-member)
parsevasp/kpoints.py:110:21: E1101: Instance of 'Kpoints' has no 'USE_ONE_ARGUMENT' member (no-member)
parsevasp/kpoints.py:220:71: E1101: Instance of 'Kpoints' has no 'ERROR_TETRA_CON_FIVE' member (no-member)
parsevasp/kpoints.py:221:41: E1101: Instance of 'Kpoints' has no 'ERROR_TETRA_CON_FIVE' member (no-member)
codecov[bot] commented 2 years ago

Codecov Report

Merging #101 (01ce018) into develop (c16cee8) will decrease coverage by 0.13%. The diff coverage is 66.60%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #101      +/-   ##
===========================================
- Coverage    80.43%   80.31%   -0.12%     
===========================================
  Files           13       13              
  Lines         3403     3381      -22     
===========================================
- Hits          2737     2715      -22     
  Misses         666      666              
Impacted Files Coverage Δ
parsevasp/constants.py 100.00% <ø> (ø)
parsevasp/kpoints.py 70.42% <46.04%> (ø)
parsevasp/poscar.py 68.77% <52.39%> (+0.30%) :arrow_up:
parsevasp/incar.py 77.84% <62.07%> (-0.10%) :arrow_down:
parsevasp/base.py 72.23% <62.50%> (-6.15%) :arrow_down:
parsevasp/doscar.py 91.09% <66.67%> (-0.42%) :arrow_down:
parsevasp/eigenval.py 91.31% <66.67%> (-0.12%) :arrow_down:
parsevasp/stream.py 88.64% <69.24%> (-2.64%) :arrow_down:
parsevasp/vasprun.py 83.29% <72.00%> (+0.21%) :arrow_up:
parsevasp/utils.py 80.59% <79.17%> (+1.64%) :arrow_up:
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c16cee8...01ce018. Read the comment docs.

atztogo commented 2 years ago

@espenfl, do we need files under the ops directory?

espenfl commented 2 years ago

@espenfl, do we need files under the ops directory?

@atztogo We do not need the run_coverals but the update_versions is nice when making a release so I would like to keep that.

atztogo commented 2 years ago

I see. Then I would like to remind the following.

pylint complained about subprocess32 module. I just changed the module name to subprocess in the file though I didn't exactly understand it. So if it causes the problem, please fix it.

https://github.com/aiida-vasp/parsevasp/blob/c16cee80a08cfcdc4fa0fd30633ece750f541243/ops/update_version.py#L7

espenfl commented 2 years ago

@atztogo Great. Thanks for adding. Also see our pre-commit config in aiida-vasp. Maybe we should try to make them pretty similar between aiida-vasp and parsevasp. Happy to do updates on the aiida-vasp side as well. Maybe we can add the yaml linter check to parsevasp as well? Also, check for large files and forbid submodules are nice, even though never strictly necessary.

espenfl commented 2 years ago

So if it causes the problem, please fix it.

Thanks for taking care of this. No, this is not an issue at all and just related to Python 2 compatibility, which is now not a concern anymore.

atztogo commented 2 years ago

Sure, but to be careful that some of them can be a full one day job, it is good after merging as another PR. And not altogether, but introducing one feature by one feature may be good for our mental health :P

I also recommend registering https://pre-commit.ci/.

espenfl commented 2 years ago

@atztogo I pushed a commit with the necessary updates. In the process I also took the liberty to convert the config to a two space indentation format and added the yaml, rst, big files and no submodules modules as we have in the aiida-vasp config. Thanks a lot for fixing this. Let me know if we think we can merge this now.

atztogo commented 2 years ago

Thank you @espenfl. For me it looks good!

atztogo commented 2 years ago

Thanks @espenfl for your quick review and merge!