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

Remove workaround for "values" being both a Mapping method name and STIX property name #331

Merged
merged 3 commits into from
Feb 4, 2020

Conversation

chisholm
Copy link
Contributor

Fixes #324 .

The workaround didn't work (caused crashes under some circumstances). Now, attributes whose names conflict with Mapping methods will have the Mapping interpretation. Same-named STIX object properties will not be accessible as attributes.

and sometimes a STIX property name.  It didn't work (caused
crashes under some circumstances).  Now, attributes whose names
conflict with Mapping methods will have the Mapping
interpretation.  Same-named STIX object properties will not be
accessible as attributes.
@codecov-io
Copy link

codecov-io commented Jan 28, 2020

Codecov Report

Merging #331 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
- Coverage   98.17%   98.12%   -0.05%     
==========================================
  Files         123      123              
  Lines       14160    13839     -321     
==========================================
- Hits        13902    13580     -322     
- Misses        258      259       +1
Impacted Files Coverage Δ
stix2/v20/observables.py 100% <ø> (ø) ⬆️
stix2/v21/observables.py 97.14% <ø> (-0.05%) ⬇️
stix2/properties.py 98.36% <100%> (-0.03%) ⬇️
stix2/test/v21/test_observed_data.py 100% <100%> (ø) ⬆️
stix2/test/v20/test_observed_data.py 100% <100%> (ø) ⬆️
stix2/datastore/filesystem.py 92.88% <0%> (-0.12%) ⬇️
stix2/patterns.py 98% <0%> (-0.02%) ⬇️
stix2/test/v21/test_datastore_filesystem.py 99.17% <0%> (-0.02%) ⬇️
stix2/test/v20/test_datastore_filesystem.py 99.19% <0%> (-0.02%) ⬇️
stix2/test/v21/test_relationship.py 100% <0%> (ø) ⬆️
... and 62 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 4b8fda0...c6b2b6f. Read the comment docs.

@clenk
Copy link
Contributor

clenk commented Jan 29, 2020

Looks good. Can we document that any properties with conflicting names can't be accessed this way? I think the place to do so would be in https://github.com/oasis-open/cti-python-stix2/blob/master/docs/guide/creating.ipynb, around in/out cells [9] where it talks about accessing the properties like standard Python attributes.

can't be used to access STIX object properties.
@chisholm
Copy link
Contributor Author

I added a cell with a prominent warning message. But notice that the notebook metadata had references to python 3 changed to python 2. Dunno if that's a problem. That happened because I used python 2 to make the change. My python 3 stix2 virtualenv was created with python 3.8, which jupyter currently has problems with. So I could either dump the whole thing and recreate with python 3.7, or just use the python 2-based virtualenv I already had. It was much easier to do the latter. But I didn't know it would have that side-effect. Does that cause any problems?

@clenk
Copy link
Contributor

clenk commented Jan 31, 2020

No, there's no issue with the python version change. But the warning gets rendered as an awkward table. Can we match the format of the warning here?

Thanks!

regarding name collisions between method and property names, to
not pick on the Mapping methods specifically.  The problem is
more general than that: stix objects have more methods than those.
Instead of listing them all out, a more general statement is
made, that accessing those attributes will result in a bound
method, not a STIX property value.
@chisholm
Copy link
Contributor Author

chisholm commented Feb 3, 2020

The table formatting problem seems to have been the fault of pandoc, which is what sphinx uses as its format converter. It has its own markdown extensions, which unfortunately result in ambiguity: it allows 3+ dashes to indicate a horizontal rule, same as standard markdown, but reuses the same syntax for its tables (several dashes above and below). Jupyter rendered it just fine, so it must use a different system for converting markdown to HTML. That is unfortunate; now we have to keep two systems happy.

Anyway, I changed it to the other format, and changed the content to not pick on the Mapping methods, since the problem is more general than that. I didn't give an exhaustive list, only said that accessing certain attributes which collide with method names will return a bound method.

Copy link
Contributor

@clenk clenk left a comment

Choose a reason for hiding this comment

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

Thanks @chisholm!

@clenk clenk merged commit c96b742 into oasis-open:master Feb 4, 2020
@emmanvg emmanvg added this to the 1.3.1 milestone Feb 4, 2020
@chisholm chisholm deleted the remove_values_workaround branch February 6, 2020 22:04
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.

STIX objects with an optional "values" property can cause crash
4 participants