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.

Sign up for GitHub

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

Jump to bottom

Convert all remaining namedtuples to attr classes #157

Closed
wants to merge 3 commits into from
Closed

Convert all remaining namedtuples to attr classes #157

wants to merge 3 commits into from

Conversation

tovrstra
Copy link
Member

@tovrstra tovrstra commented Jul 9, 2020 •
edited
Loading

Fixes #201 amongst other things:

  • Improved attribute validation, both in constructor and upon attribute assignment (requires latest attrs version).
  • attrutil module to facilitate writing validators.
  • Fixed bug in CP2K loader.
  • Document attr behavior in CONTRIBUTING.rst
  • Few other fixes needed to satisfy validators.

@tovrstra tovrstra requested a review from FarnazH July 9, 2020 13:38
@tovrstra tovrstra mentioned this pull request Jul 9, 2020
Closed
@tovrstra
Copy link
Member Author

tovrstra commented Jul 9, 2020

The motivation for the changes in this PR are outlined in more detail in #138. Most changes from that PR were also used here, but with some modifications and corrections.

@tovrstra
Copy link
Member Author

@FarnazH I removed the code again that explicitly runs the validators before dumping and after loading. Even without these, just by turning Shell into an attr class and adding validators, the bug in cp2klog.py already raises an error when constructing the Shell instance. Hence, running the validators once more seemed overkill. As soon as tests pass, it is ready for review.

@tovrstra tovrstra mentioned this pull request Jul 28, 2020
Closed
@tovrstra tovrstra added the API breaking Should be done first to stabilize API label Aug 16, 2020
@tovrstra tovrstra mentioned this pull request Aug 23, 2020
Closed
@tovrstra
Copy link
Member Author

@FarnazH One thing we should consider adding here is to update all NamedTuple classes into attr classes. This may resolve issues like the one you mentioned here: #188 (comment)

@tovrstra tovrstra mentioned this pull request Aug 27, 2020
Closed
@tovrstra tovrstra force-pushed the attribute_validation branch from e2ec8c3 to db297ac Compare August 27, 2020 20:31
@tovrstra
Copy link
Member Author

@wilhadams This PR switches all namedtuples to attr classes, adding more validation and a few minor bug fixes. Does the last section in CONTRIBUTING.rst address your concern?

@tovrstra tovrstra requested a review from wilhadams August 27, 2020 20:35
wilhadams
wilhadams previously approved these changes Aug 28, 2020
View reviewed changes
CONTRIBUTING.rst Show resolved Hide resolved
@tovrstra tovrstra force-pushed the attribute_validation branch from 9af6cef to cda1f95 Compare August 29, 2020 07:47
@tovrstra tovrstra requested a review from wilhadams August 29, 2020 07:48
@tovrstra tovrstra changed the title Add attribute validation to Shell class Convert all remaining namedtuples to attr classes Sep 2, 2020
@tovrstra
Turn all NamedTuple classes into attr classes
0b3cdad 
This introduces additional attribute validation, which fixes some
silly bugs.
@tovrstra
Remove evolve methods
b718c26
@tovrstra
Attribute validation on assignment
9b66202
@tovrstra tovrstra force-pushed the attribute_validation branch from cda1f95 to 9b66202 Compare September 2, 2020 14:17
@codecov
Copy link

codecov bot commented Sep 2, 2020 •
edited
Loading

Codecov Report

Merging #157 into master will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #157      +/-   ##
==========================================
+ Coverage   95.56%   95.65%   +0.09%     
==========================================
  Files          61       63       +2     
  Lines        6605     6743     +138     
  Branches      847      865      +18     
==========================================
+ Hits         6312     6450     +138     
  Misses        136      136              
  Partials      157      157
Impacted Files Coverage Δ
iodata/formats/chgcar.py 98.14% <ø> (ø)
iodata/attrutils.py 100.00% <100.00%> (ø)
iodata/basis.py 100.00% <100.00%> (ø)
iodata/formats/cp2klog.py 97.54% <100.00%> (ø)
iodata/formats/cube.py 100.00% <100.00%> (ø)
iodata/formats/molden.py 91.32% <100.00%> (+0.02%) ⬆️
iodata/iodata.py 100.00% <100.00%> (ø)
iodata/orbitals.py 68.29% <100.00%> (ø)
iodata/overlap.py 100.00% <100.00%> (ø)
iodata/test/test_attrutils.py 100.00% <100.00%> (ø)
... and 8 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 92961d6...9b66202. Read the comment docs.

@tovrstra
Copy link
Member Author

tovrstra commented Sep 3, 2020

I'm going to close this PR and reopen it because most of the comments above are outdated.

@tovrstra tovrstra closed this Sep 3, 2020
tovrstra added a commit to tovrstra/iodata that referenced this pull request Sep 3, 2020
@tovrstra
Turn all numedtuples into attrs classes
e0d7f6a 
Fixes theochem#201

Related to theochem#138 and theochem#157 (which were earlier attempts)

This PR includes:
- Attribute validation (to large extent, not every detail)
- attrutil module to facilitate validation of array attributes
- Documentation of how attrs is used in IOData
- Bug fix in CP2K loader
- Minor fixes elsewhere
tovrstra added a commit to tovrstra/iodata that referenced this pull request Sep 3, 2020
@tovrstra
Turn all numedtuples into attrs classes
54bb66a 
Fixes theochem#201

Related to theochem#138 and theochem#157 (which were earlier attempts)

This PR includes:
- Attribute validation (to large extent, not every detail)
- attrutil module to facilitate validation of array attributes
- Documentation of how attrs is used in IOData
- Bug fix in CP2K loader
- Minor fixes elsewhere
tovrstra added a commit to tovrstra/iodata that referenced this pull request Sep 3, 2020
@tovrstra
Turn all numedtuples into attrs classes
13b841d 
Fixes theochem#201

Related to theochem#138 and theochem#157 (which were earlier attempts)

This PR includes:
- Attribute validation (to large extent, not every detail)
- attrutil module to facilitate validation of array attributes
- Documentation of how attrs is used in IOData
- Bug fix in CP2K loader, related to theochem/gbasis#78
- Minor fixes elsewhere to satisfy attribute validators
@tovrstra tovrstra mentioned this pull request Sep 3, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FarnazH FarnazH Awaiting requested review from FarnazH

@wilhadams wilhadams Awaiting requested review from wilhadams

Assignees
No one assigned
Labels
API breaking Should be done first to stabilize API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Document required attrs package
2 participants
@tovrstra @wilhadams