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

fp write for STIX Objects #500

Merged
merged 5 commits into from
Mar 20, 2021
Merged

Conversation

emmanvg
Copy link
Contributor

@emmanvg emmanvg commented Mar 17, 2021

Adding this piece of code would help write more flexible code while also providing the same options as serialize()

@emmanvg emmanvg added this to the 3.0.0 milestone Mar 17, 2021
Copy link
Contributor

@chisholm chisholm left a comment

Choose a reason for hiding this comment

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

I like this idea. Could the serialize() function even be reimplemented to call fp_serialize() and pass an io.StringIO? That would remove the code duplication between the two functions.

stix2/serialization.py Outdated Show resolved Hide resolved
@emmanvg
Copy link
Contributor Author

emmanvg commented Mar 18, 2021

I started with having both under a same function, but I was doubtful about having two different behaviors (one that returned a string and one that returned None). Based on the same idea that json.dumps() and json.dump() are essentially the same except for the required fp argument and what each return, I thought it would be better to also keep them separate on stix2 too.

@codecov-io
Copy link

codecov-io commented Mar 18, 2021

Codecov Report

Merging #500 (c2d360d) into master (f155e3e) will decrease coverage by 0.00%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #500      +/-   ##
==========================================
- Coverage   89.47%   89.47%   -0.01%     
==========================================
  Files         147      147              
  Lines       16554    16599      +45     
==========================================
+ Hits        14812    14852      +40     
- Misses       1742     1747       +5     
Impacted Files Coverage Δ
stix2/base.py 74.70% <50.00%> (-0.20%) ⬇️
stix2/datastore/filesystem.py 75.90% <66.66%> (-0.10%) ⬇️
stix2/serialization.py 70.96% <71.42%> (-0.97%) ⬇️
stix2/test/v20/test_bundle.py 100.00% <100.00%> (ø)
stix2/test/v21/test_bundle.py 100.00% <100.00%> (ø)
stix2/datastore/taxii.py 69.60% <0.00%> (-0.15%) ⬇️
stix2/test/v20/test_datastore_taxii.py 100.00% <0.00%> (ø)
stix2/test/v21/test_datastore_taxii.py 100.00% <0.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 f155e3e...c2d360d. Read the comment docs.

@chisholm
Copy link
Contributor

I didn't mean to combine them into one function. It was more like change the serialize() function to be something like:

with io.StringIO() as fp:
    fp_serialize(obj, fp)
    return fp.getvalue()

So the "main" code for serializing lives in one place, the two functions can't get out of sync, the serialize() implementation is shorter and simpler, etc. Just a thought.

@emmanvg
Copy link
Contributor Author

emmanvg commented Mar 18, 2021

I misinterpreted your prior message. It is a good suggestion...

stix2/test/v20/test_bundle.py Outdated Show resolved Hide resolved
stix2/test/v20/test_bundle.py Outdated Show resolved Hide resolved
stix2/test/v21/test_bundle.py Outdated Show resolved Hide resolved
stix2/test/v21/test_bundle.py Outdated Show resolved Hide resolved
Co-authored-by: Chris Lenk <clenk@users.noreply.github.com>
@clenk clenk merged commit 2743b90 into oasis-open:master Mar 20, 2021
@emmanvg emmanvg deleted the add-fp-serialize branch March 22, 2021 13:10
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