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

Document uniqueness of name on Organization and User #7

Merged
merged 6 commits into from
Nov 5, 2023

Conversation

nicksnyder
Copy link
Member

@nicksnyder nicksnyder commented Oct 30, 2023

Document the uniqueness of the name field on the field itself.

Closes #2

@nicksnyder nicksnyder requested a review from bufdev as a code owner October 30, 2023 18:28
@bufdev
Copy link
Member

bufdev commented Oct 30, 2023

I could go either way on the meat of this - moving the unique fields above the create_time and update_time. The thinking was that the "standard resource fields", namely id, create_time, update_time, all come first, where the resource-specific fields come later.

If we did do this, you'd need to update the comments globally, not just for one package (ie all of the module package needs to be updated as well). However, I do think having the uniqueness constraints at the top level on a message is nice, so we could perhaps have them both on the field and at the top level.

@nicksnyder
Copy link
Member Author

However, I do think having the uniqueness constraints at the top level on a message is nice, so we could perhaps have them both on the field and at the top level.

I updated this PR to just be about copying the documentation for uniqueness on the field itself. Can consider re-ordering fields in a separate PR.

@nicksnyder
Copy link
Member Author

The thinking was that the "standard resource fields", namely id, create_time, update_time, all come first, where the resource-specific fields come later.

Yeah, I figured.

If we did do this, you'd need to update the comments globally, not just for one package (ie all of the module package needs to be updated as well).

Not sure what you mean here. It isn't clear to me that we would want to re-order name field in branch, module, tag because those are only identifiers within some context (user/org/module), and not really equivalent to an id or name in the same sense that it is for User and Organization.

In any case, after some reflection, I think adding docs to the field is sufficient (which is what this PR diff now is), and we don't need to go so far as changing the order of or renaming the fields.

@bufdev
Copy link
Member

bufdev commented Oct 30, 2023

Not sure what you mean here.

Yea looking into it, we actually do have "uniqueness" comments on the module package types, I wasn't paying attention closely enough. This LGTM.

@nicksnyder nicksnyder enabled auto-merge (squash) October 30, 2023 23:28
@nicksnyder nicksnyder merged commit 6e616aa into main Nov 5, 2023
6 checks passed
@nicksnyder nicksnyder deleted the nameuniqueness branch November 5, 2023 20:28
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.

Better represent/document uniqueness of name on User and Organization
2 participants