Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add resource type scalar in
typespec-azure-core
#1272base: main
Are you sure you want to change the base?
add resource type scalar in
typespec-azure-core
#1272Changes from 8 commits
b3c0834
a95653d
ac93669
de619ce
a3c466c
2b68cf3
3494df5
47aa996
914fb65
840aa34
a1a3730
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was OOF the last 2 weeks, has there been agreement we needed this and this was the name we wanted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh - I do not know. Do we need to bring up this topic in one of our typespec sync up meeting or your internal typespec sync up meeting?
In those meetings that I have participated in, this has not been a topic there.
Or maybe we could put this as an agenda in our next typespec sync up meeting on your wednesday night.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah would be good, do you need this for this release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we probably need to differentiate this as we did with armResourceIdentifier - armResourceType.
There is definitely use for this in ARM, and since the type is needed to properly specify the allowedResourceType in armResourceIdentifier, it seems appropriate to put this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree the name should be
armResourceType
which makes more sense.The second part is that you suggest we add a generic argument for allowed resource type? like this?
I think this looks weird because this type is modeling the resource type of myself, if there is a limited list of resource types that this field is supporting, should we define a extensible enum based on resource type like this?
Well but I could see values for this because we could update the definition of
Resource
andTrackedResource
to let them accept another argument of resource type like this:so that we finally have explicitly defined resource type on resources, currently arm resources could have resource types but they are implicitly - we can infer them from the uri paths we built for resource related operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think using a union here instead to be more precise makes more sense instead of the
armREsourceIdentifier
which does that for a subset of the value.and
+1
on the name beingarmResourceType