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

update dict key limits per 2.1 spec #198

Merged
merged 6 commits into from
Jun 26, 2018
Merged

Conversation

treyka
Copy link
Contributor

@treyka treyka commented Jun 26, 2018

In the course of testing the i18n code in the 2.1 branch, I noticed that while the language-content contents property allows 2 char dict keys for specifying languages (as shown in all of the examples and per RFC5646), the 2.1 branch still retained the 2.0 3 char restriction on dict key length.

QED, I split properties.py out v20 vs v21 as was done previously to accommodate other 2.1 changes.

Also, somewhat inexplicably the STIX2 dictionary property limits key length to 250 (not 256). While I was mucking about, I fixed this on for 2.1.

@emmanvg emmanvg self-assigned this Jun 26, 2018
@codecov-io
Copy link

Codecov Report

Merging #198 into stix2.1 will decrease coverage by 1.27%.
The diff coverage is 90.23%.

Impacted file tree graph

@@             Coverage Diff             @@
##           stix2.1     #198      +/-   ##
===========================================
- Coverage    97.06%   95.79%   -1.28%     
===========================================
  Files           75       76       +1     
  Lines         7525     7745     +220     
===========================================
+ Hits          7304     7419     +115     
- Misses         221      326     +105
Impacted Files Coverage Δ
stix2/base.py 99.47% <0%> (ø) ⬆️
stix2/test/test_language_content.py 100% <100%> (+21.42%) ⬆️
stix2/v20/observables.py 53.89% <100%> (-14.02%) ⬇️
stix2/v21/sdo.py 100% <100%> (ø) ⬆️
stix2/v21/sro.py 100% <100%> (ø) ⬆️
stix2/v21/observables.py 100% <100%> (ø) ⬆️
stix2/core.py 100% <100%> (ø) ⬆️
stix2/v20/properties.py 90.13% <100%> (ø)
stix2/v21/bundle.py 100% <100%> (ø) ⬆️
stix2/v20/bundle.py 39.47% <100%> (ø) ⬆️
... and 15 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 b852b91...59fdd30. Read the comment docs.

@emmanvg
Copy link
Contributor

emmanvg commented Jun 26, 2018

@treyka thanks for updating the properties and make LanguageContent work. I removed the mark.xfail() for the test to make sure it passes now. I saw that you created properties.py for both versions, but honestly most of the code is duplicated. We will revisit this in #193. I had to make a minimal amount of changes to the tests to make sure they pass. We will revisit this for #197 and #80.

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.

There is more stuff to be done, but we can address in a different PR.

@emmanvg emmanvg merged commit 9cc74e8 into oasis-open:stix2.1 Jun 26, 2018
@gtback
Copy link
Contributor

gtback commented Jun 27, 2018

Thanks, @treyka !

@gtback gtback added this to the STIX 2.1 milestone Jun 27, 2018
@emmanvg emmanvg modified the milestones: 2.0.0, 1.1.0 Oct 31, 2018
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