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

addition of asset.type #261

Merged
merged 2 commits into from
Aug 5, 2024
Merged

addition of asset.type #261

merged 2 commits into from
Aug 5, 2024

Conversation

pisuke
Copy link
Collaborator

@pisuke pisuke commented Jul 17, 2024

Addition of asset.type as discussed in #258 and #260

@RitaLav
Copy link
Collaborator

RitaLav commented Jul 17, 2024

@jgunstone @TheFridgeShaman @RitaLav

To review and add comments

@RitaLav
Copy link
Collaborator

RitaLav commented Jul 17, 2024

Thought for consideration:

  • Repeat that Device/Asset type (asset.type) is entirely optional anywhere where it is mentioned so that the fact that it is optional is not missed or misunderstood.

jgunstone
jgunstone requested changes Jul 18, 2024
View reviewed changes
Copy link
Collaborator

@jgunstone jgunstone left a comment

Choose a reason for hiding this comment

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

hi - apologies I didn't make the meeting yesterday - I was fire-fighting some server issues! This looks great to me, I've made a minor comment about incorporating Device/Asset Type in the header definintions. Many Thanks

BDNS_Specification_naming_syntax.md Show resolved Hide resolved
BDNS_Specification_naming_syntax.md Outdated Show resolved Hide resolved
@pisuke pisuke requested review from jgunstone and RitaLav July 24, 2024 10:57
@RitaLav RitaLav requested a review from TheFridgeShaman July 25, 2024 10:14
@RitaLav
Copy link
Collaborator

RitaLav commented Jul 31, 2024

@jgunstone we have reviewed and approved - if you are happy with the PR we can merge to master.

@RitaLav
Copy link
Collaborator

RitaLav commented Jul 31, 2024

Once merged we can also do a version release.

jgunstone
jgunstone approved these changes Aug 5, 2024
View reviewed changes
Copy link
Collaborator

@jgunstone jgunstone left a comment

Choose a reason for hiding this comment

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

this looks great and @pisuke incorporated the comments I had -
sorry for holding this up - I was on holiday last week.

thanks everyone

@jgunstone jgunstone merged commit 31e5b40 into theodi:master Aug 5, 2024
1 check passed
@RitaLav RitaLav mentioned this pull request Oct 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RitaLav RitaLav RitaLav approved these changes

@TheFridgeShaman TheFridgeShaman TheFridgeShaman approved these changes

@jgunstone jgunstone jgunstone approved these changes

Assignees

@jgunstone jgunstone

@TheFridgeShaman TheFridgeShaman

@RitaLav RitaLav

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pisuke @RitaLav @jgunstone @TheFridgeShaman