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.

Sign up for GitHub

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

Jump to bottom

Change property name imports to import #847

Merged
merged 4 commits into from
Aug 13, 2024
Merged

Change property name imports to import #847

merged 4 commits into from
Aug 13, 2024

Conversation

goneall
Copy link
Member

@goneall goneall commented Aug 12, 2024

All property names should be singular

@goneall
Change property name imports to import
b5092b1 
All property names should be singular

Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall goneall requested review from zvr, bact and kestewart August 12, 2024 18:48
@goneall goneall added this to the 3.0.1 milestone Aug 12, 2024
kestewart
kestewart approved these changes Aug 12, 2024
View reviewed changes
Copy link
Contributor

@kestewart kestewart left a comment

Choose a reason for hiding this comment

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

Urk, another one. Find this one from the tooling?

bact
bact approved these changes Aug 12, 2024
View reviewed changes
Copy link
Collaborator

@bact bact left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approved for the code in here.
Need to update a related example in serialization/ afterward.

spdx-3-model/serialization/jsonld/examples/converted_from_spdx_2.json

Line 96 in 647f94b

"imports": [

zvr
zvr requested changes Aug 12, 2024
View reviewed changes
Copy link
Member

@zvr zvr left a comment

Choose a reason for hiding this comment

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

I agree, but then the description of "import" should refer to only one entry.

model/Core/Properties/import.md Outdated Show resolved Hide resolved
bobmartin3000
bobmartin3000 approved these changes Aug 12, 2024
View reviewed changes
Copy link

@bobmartin3000 bobmartin3000 left a comment

Choose a reason for hiding this comment

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

LGTM

@goneall @zvr
Fix grammar
cf490ac 
Co-authored-by: Alexios Zavras (zvr) <github@zvr.gr>
Signed-off-by: Gary O'Neall <gary@sourceauditor.com>
@goneall
Copy link
Member Author

goneall commented Aug 13, 2024

Urk, another one. Find this one from the tooling?

Yes - I've implemented the code to translate from 2.X to 3.X - this was found from the unit testing. Should be done with the unit tests by the end of the week.

@bact bact mentioned this pull request Aug 13, 2024
Merged
@bact
Copy link
Collaborator

bact commented Aug 13, 2024

@goneall I have add my suggestions to #848

@bact @goneall
Add change log
e598be8 
Signed-off-by: Arthit Suriyawongkul <arthit@gmail.com>
@bact @goneall
3.0.0 -> 3.0.1
f1fcbcb 
Signed-off-by: Arthit Suriyawongkul <arthit@gmail.com>
kestewart
kestewart approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@kestewart kestewart left a comment

Choose a reason for hiding this comment

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

Re-reviewed. Changes look good. Going ahead with merging.

@kestewart kestewart merged commit f12a54c into main Aug 13, 2024
1 check passed
@bact bact deleted the imports-fix branch August 13, 2024 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zvr zvr zvr requested changes

@bact bact bact approved these changes

@kestewart kestewart kestewart approved these changes

@bobmartin3000 bobmartin3000 bobmartin3000 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.1
Development

Successfully merging this pull request may close these issues.
5 participants
@goneall @bact @zvr @kestewart @bobmartin3000