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

fix: Respect name of @Custom* decorated defs #439

Merged
merged 1 commit into from
Aug 4, 2020

Conversation

maybe-sybr
Copy link
Contributor

This change makes it easier to refer to @Custom* decorated classes programmatically.

@maybe-sybr
Copy link
Contributor Author

This is a fairly minor change but if you need a CLA signed, let me know. Thanks!

@clenk
Copy link
Contributor

clenk commented Jul 27, 2020

Hi @maybe-sybr. Thank you for the submission, it's a good change. Would you mind fixing the unit tests though: https://travis-ci.org/github/oasis-open/cti-python-stix2/builds/712041095. If you're able to sign a CLA that would be awesome: https://www.oasis-open.org/resources/open-repositories/cla/individual-cla

Thanks again!

emmanvg pushed a commit that referenced this pull request Jul 27, 2020
... if the object type hasn't been registered.

Related: #439.
@maybe-sybr maybe-sybr force-pushed the fix/customs-class-name branch from 87f78b9 to 1534452 Compare July 28, 2020 01:14
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2020

Codecov Report

Merging #439 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #439   +/-   ##
=======================================
  Coverage   98.38%   98.39%           
=======================================
  Files         130      131    +1     
  Lines       14906    14942   +36     
=======================================
+ Hits        14666    14702   +36     
  Misses        240      240           
Impacted Files Coverage Δ
stix2/custom.py 100.00% <100.00%> (ø)
stix2/test/v20/test_custom.py 100.00% <100.00%> (ø)
stix2/test/v21/test_custom.py 100.00% <100.00%> (ø)
stix2/__init__.py 100.00% <0.00%> (ø)
stix2/test/v20/test_utils.py 100.00% <0.00%> (ø)
stix2/test/v21/test_utils.py 100.00% <0.00%> (ø)
stix2/serialization.py 94.73% <0.00%> (ø)
stix2/test/v21/test_datastore_filesystem.py 99.23% <0.00%> (+0.01%) ⬆️
stix2/test/v20/test_datastore_filesystem.py 99.21% <0.00%> (+0.01%) ⬆️
stix2/base.py 96.85% <0.00%> (+0.09%) ⬆️
... and 2 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 bcd3eb3...1534452. Read the comment docs.

@maybe-sybr
Copy link
Contributor Author

maybe-sybr commented Jul 28, 2020

@clenk I've sent in a CLA, lmk if there are any issues. In the meantime, I've also fixed up those two unit tests which were failing and CI went green at https://travis-ci.org/github/oasis-open/cti-python-stix2/builds/712398792 .

I did notice that the class names were only explicitly referenced for CustomExtensions and more as a side-effect of another behavioural test. Is it at all useful to add tests specifically for properties of the derived class objects? Since we're not passing up things like __doc__ as well, we're still not really wrapping the decorated class properly, but that may also not be a thing we particularly care about.

@clenk
Copy link
Contributor

clenk commented Aug 4, 2020

Thanks, @maybe-sybr your CLA's on file. Yes, it would be useful to have tests for these properties specifically. Properly wrapping the decorated class and passing up all of the attributes hasn't been a priority for us but would be an improvement. We'd accept any pull requests to that end, though! :)

@clenk clenk merged commit 1f9a844 into oasis-open:master Aug 4, 2020
@emmanvg emmanvg added this to the 2.1.0 milestone Aug 4, 2020
@maybe-sybr maybe-sybr deleted the fix/customs-class-name branch August 6, 2020 07:02
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.

4 participants