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

protect keys from writing/deleting/manipulating #625

Merged
merged 28 commits into from Nov 9, 2021

Conversation

marscher
Copy link
Collaborator

@marscher marscher commented Nov 2, 2021

Changes

Protect some keys from reading/writing/deleting/manipulating them. There is a list of attributes in file.py, which are being passed to the protected key dictionary. The WeldxFile now derives from it.
We need to deprecate the data attribute, as it allowed raw access.

Related Issues

Closes #557

Checks

  • updated CHANGELOG.rst
  • updated tests
  • update example/tutorial notebooks

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #625 (56b58ad) into master (bb1e2c3) will decrease coverage by 0.03%.
The diff coverage is 91.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #625      +/-   ##
==========================================
- Coverage   94.52%   94.49%   -0.04%     
==========================================
  Files          93       93              
  Lines        5959     6011      +52     
==========================================
+ Hits         5633     5680      +47     
- Misses        326      331       +5     
Impacted Files Coverage Δ
weldx/asdf/util.py 89.45% <88.88%> (-0.39%) ⬇️
weldx/asdf/file.py 95.98% <95.65%> (+0.47%) ⬆️
weldx/asdf/cli/welding_schema.py 96.90% <100.00%> (ø)

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 bb1e2c3...56b58ad. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Nov 2, 2021

Unit Test Results

       1 files  ±0         1 suites  ±0   1m 48s ⏱️ ±0s
1 930 tests +5  1 930 ✔️ +5  0 💤 ±0  0 ±0 

Results for commit 56b58ad. ± Comparison against base commit bb1e2c3.

♻️ This comment has been updated with latest results.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@marscher marscher marked this pull request as ready for review November 3, 2021 10:11
@marscher
Copy link
Collaborator Author

marscher commented Nov 5, 2021

Does this "arguments differ" warning raised by codacy origin from pylint? I cannot figure out which tool they use. The argument has been renamed from the top-level typing interface named "__m" to "mapping" for better user-experience. I can also add an ignore for this.
Another possibility which might have triggered this is the different (more sane) type annotation. I would also refuse to change that, because it makes more sense to require a key to be hashable in contrast to the super broad TypeVar("_K").

@CagtayFabry
Copy link
Member

it looks like the pylint run but I am fine with this kind of error (or adding a simple ignore 👍 )

@vhirtham
Copy link
Collaborator

vhirtham commented Nov 5, 2021

If you expand the error box, the second line is:

Reported by Pylint (Python 3) Time to fix: 5 minutes

So as @CagtayFabry said, its pylint. Just add # pylint: disable=W0221 if you think we can ignore the issue

Copy link
Member

@CagtayFabry CagtayFabry left a comment

Choose a reason for hiding this comment

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

great, thanks! 🚀

weldx/asdf/file.py Outdated Show resolved Hide resolved
weldx/asdf/util.py Outdated Show resolved Hide resolved
Co-authored-by: vhirtham <volker.hirthammer@gmail.com>
@marscher
Copy link
Collaborator Author

marscher commented Nov 9, 2021

these "kernel died" errors during the notebook tests seem unrelated to the latest change.

@vhirtham
Copy link
Collaborator

vhirtham commented Nov 9, 2021

these "kernel died" errors during the notebook tests seem unrelated to the latest change.

Seems so, just restart the actions and see what happens (the simplest way is updating the branch. You have to do it anyways)

@marscher
Copy link
Collaborator Author

marscher commented Nov 9, 2021

Now the number of hacks involved to get or set protected keys has been increased further (just for you @vhirtham :D)

@marscher marscher merged commit 299d199 into BAMWelDX:master Nov 9, 2021
@marscher marscher deleted the wxfile_protected_keys branch November 9, 2021 14:47
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.

weldxfile: consider removing asdf metadata from user dictionary like operations.
3 participants