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

Add Hvendor and Hname as attributes of accept, require and deny elements #228

Merged
merged 2 commits into from
Apr 12, 2023
Merged

Add Hvendor and Hname as attributes of accept, require and deny elements #228

merged 2 commits into from
Apr 12, 2023

Conversation

silviooliva
Copy link
Contributor

@silviooliva silviooliva commented Apr 12, 2023 •
edited
Loading

Commit related to issue #182. This allows part specific filtering.

@silviooliva
Add Hvendor and Hname as attributes of accept, require and deny elements
51ccb6f 
Commit related to issue Open-CMSIS-Pack#182:
Open-CMSIS-Pack#182
This allows part specific filtering.

Signed-off-by: Silvio Lucio Oliva <silvio.oliva@st.com>
@silviooliva silviooliva changed the title Add Hvendor and Hname as attributes of accept, require and deny elements Add Hvendor and Hname as attributes of accept, require and deny elements Apr 12, 2023
jkrech
jkrech reviewed Apr 12, 2023
View reviewed changes
schema/PACK.xsd Outdated Show resolved Hide resolved
jkrech
jkrech requested changes Apr 12, 2023
View reviewed changes
Copy link
Member

@jkrech jkrech left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR.
Please see my comment. Are you happy to change to Hvendor to xs:string. This is similar to Bvendor where we don't have an fixed list with vendorID numbers.

@silviooliva
Copy link
Contributor Author

Thanks a lot for the PR. Please see my comment. Are you happy to change to Hvendor to xs:string. This is similar to Bvendor where we don't have an fixed list with vendorID numbers.

@jkrech,
I've used the DeviceVendorEnum since the the Hvendor has been defined as DeviceVendorEnum type (see the part element description).
Changing the Hvendor here to xs:string, maybe we should also change the Hvendor attribute definition in the part element (no problem on my side to fix it as well). Or do you think we could keep the two definitions disaligned?

@jkrech
Copy link
Member

jkrech commented Apr 12, 2023

@silviooliva, good catch. I do agree that we should also make it xs:string for the part element:
https://open-cmsis-pack.github.io/Open-CMSIS-Pack-Spec/main/html/pdsc_parts_pg.html#element_part
Could you please do so?

@silviooliva
Copy link
Contributor Author

@jkrech,
after further thinking, I'm convinced too it is better, for the same reason you exposed here, to use xs:string for the Hvendor.
I'm going to fix it.

@jkrech jkrech linked an issue Apr 12, 2023 that may be closed by this pull request
Add Hvendor and Hname as attributes of <accept>, <require> and <deny> elements #182
Closed
@silviooliva
Redefine Hvendor as xs:string
a3d265b 
Using an xs:string will not require maintaining a list of vendors.

Signed-off-by: Silvio Lucio Oliva <silvio.oliva@st.com>
@silviooliva silviooliva requested a review from jkrech April 12, 2023 15:09
jkrech
jkrech approved these changes Apr 12, 2023
View reviewed changes
Copy link
Member

@jkrech jkrech left a comment

Choose a reason for hiding this comment

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

Great. Thanks for making updates in all affected locations/files. LGTM.

@jkrech jkrech merged commit 537969d into Open-CMSIS-Pack:main Apr 12, 2023
@silviooliva silviooliva deleted the add_Hvendor_and_Hname_as_attributes_of_accept_require_and_deny_elements branch April 12, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkrech jkrech jkrech approved these changes

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

Successfully merging this pull request may close these issues.

Add Hvendor and Hname as attributes of <accept>, <require> and <deny> elements
2 participants
@silviooliva @jkrech