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

#926 Initialize assy metadata #928

Merged
merged 4 commits into from
Dec 10, 2021
Merged

#926 Initialize assy metadata #928

merged 4 commits into from
Dec 10, 2021

Conversation

gumyr
Copy link
Contributor

@gumyr gumyr commented Dec 3, 2021

With this change metadata is a full instance variable of the Assembly class with a default of None. The _copy() method was changed to copy metadata ensuring that metadata added in a sub-assembly is maintained.

A new test verifies that the metadata is present and editable in both the base and sub-assembly.

@gumyr
Copy link
Contributor Author

gumyr commented Dec 4, 2021

As implemented here the initialization is to None as this is slightly more memory efficient than an empty dict {}. However, this makes working with metadata a little more complex as elements can't be added until it's created which requires an if / then. Adam probably anticipated this - what do you think of initialization to {}?

@codecov
Copy link

codecov bot commented Dec 4, 2021

Codecov Report

Merging #928 (3cdb43c) into master (5f65afd) will decrease coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 3cdb43c differs from pull request most recent head fb1c302. Consider uploading reports for the commit fb1c302 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #928      +/-   ##
==========================================
- Coverage   96.11%   96.05%   -0.06%     
==========================================
  Files          39       39              
  Lines        9130     9162      +32     
  Branches     1017     1107      +90     
==========================================
+ Hits         8775     8801      +26     
- Misses        206      209       +3     
- Partials      149      152       +3     
Impacted Files Coverage Δ
cadquery/assembly.py 93.46% <100.00%> (+0.16%) ⬆️
tests/test_assembly.py 100.00% <100.00%> (ø)
cadquery/sketch.py 96.25% <0.00%> (-1.43%) ⬇️

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 5f65afd...fb1c302. Read the comment docs.

@gumyr
Copy link
Contributor Author

gumyr commented Dec 7, 2021

Does anyone know what the problem is here? The code is working in my build.

@jmwright
Copy link
Member

jmwright commented Dec 8, 2021

Does anyone know what the problem is here? The code is working in my build.

The glibc error in the Linux builds is effecting another PR and will be addressed. You can ignore the codecov failure. It is over-sensitive sometimes.

@adam-urbanczyk
Copy link
Member

Not related to this PR @gumyr (which BTW looks good). I'm trying to debug this (CI) issue in #926.

Copy link
Member

@jmwright jmwright left a comment

Choose a reason for hiding this comment

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

Thanks @gumyr !

@adam-urbanczyk
Copy link
Member

Merging, thanks for the contribution @gumyr !

@adam-urbanczyk adam-urbanczyk merged commit 84edaf5 into CadQuery:master Dec 10, 2021
@jmwright
Copy link
Member

All green now with the 18.04 fix and it has been approved, so I'm going to merge.

@jmwright
Copy link
Member

Looks like @adam-urbanczyk beat me to it. Thanks @gumyr

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.

Initialize assy metadata to {}
3 participants