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 bug when adding custom object to FileSystemSink if the object type hasn't been registered #431

Merged
merged 8 commits into from
Jul 27, 2020

Conversation

clenk
Copy link
Contributor

@clenk clenk commented Jul 20, 2020

Related: #429.

... if the object type hasn't been registered.

Related: #439.
@codecov-commenter
Copy link

codecov-commenter commented Jul 20, 2020

Codecov Report

Merging #431 into master will increase coverage by 0.00%.
The diff coverage is 96.90%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #431   +/-   ##
=======================================
  Coverage   98.37%   98.37%           
=======================================
  Files         130      131    +1     
  Lines       14886    14936   +50     
=======================================
+ Hits        14644    14694   +50     
  Misses        242      242           
Impacted Files Coverage Δ
stix2/__init__.py 100.00% <ø> (ø)
stix2/utils.py 96.66% <ø> (+0.74%) ⬆️
stix2/serialization.py 94.73% <94.73%> (ø)
stix2/base.py 96.85% <100.00%> (+0.09%) ⬆️
stix2/datastore/filesystem.py 94.42% <100.00%> (+0.11%) ⬆️
stix2/test/v20/test_datastore_filesystem.py 99.21% <100.00%> (+0.01%) ⬆️
stix2/test/v20/test_utils.py 100.00% <100.00%> (ø)
stix2/test/v21/test_datastore_filesystem.py 99.23% <100.00%> (+0.01%) ⬆️
stix2/test/v21/test_utils.py 100.00% <100.00%> (ø)
stix2/v20/bundle.py 100.00% <0.00%> (ø)
... 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 c497038...08137ff. Read the comment docs.

@emmanvg
Copy link
Contributor

emmanvg commented Jul 22, 2020

General comment for this PR is that serialization logic being removed from _STIXBase and being standalone it can follow a similar path to the versioning.py or parsing.py modules. Perhaps we could move the serialize, STIXJSONEncoder and the STIXJSONIncludeOptionalDefaultsEncoder to their own separate module.

This would also remove the them from the __all__ override under base.py

@emmanvg emmanvg self-requested a review July 22, 2020 15:24
@chisholm
Copy link
Contributor

While you're at it... what about the find_property_index() stuff from utils? It's only used for pretty-serialization. Maybe it belongs in serialization.py too.

@emmanvg
Copy link
Contributor

emmanvg commented Jul 22, 2020

Agree @chisholm, will make the change

@clenk
Copy link
Contributor Author

clenk commented Jul 27, 2020

These changes look good to me!

Copy link
Contributor

@emmanvg emmanvg left a comment

Choose a reason for hiding this comment

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

LGTM with overall changes.

@emmanvg emmanvg added this to the 2.1.0 milestone Jul 27, 2020
@emmanvg emmanvg merged commit 8cdbfed into master Jul 27, 2020
@emmanvg emmanvg deleted the filesys-write-custom branch July 27, 2020 13:44
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