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

Add API docs for owners' template resource. #3282

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

martinrrm
Copy link
Contributor

@martinrrm martinrrm commented Apr 15, 2024

@martinrrm martinrrm requested review from a team as code owners April 15, 2024 17:36
Copy link

Learn Build status updates of commit b8de56d:

⚠️ Validation status: warnings

File Status Preview URL Details
docs/TOC.md ⚠️Warning View Details
docs/api/overview.md ✅Succeeded View
docs/api/owner-details-template-resource.md ✅Succeeded View

docs/TOC.md

  • Line 128, Column 6: [Warning: file-not-found - See documentation] Invalid file link: 'api/package-owner-template-resource.md'.

For more details, please refer to the build report.

Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

docs/TOC.md Outdated Show resolved Hide resolved
@@ -0,0 +1,68 @@
---
Copy link
Member

Choose a reason for hiding this comment

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

This doc LGTM.

Copy link

Learn Build status updates of commit 7d33f49:

⚠️ Validation status: warnings

File Status Preview URL Details
docs/TOC.md ⚠️Warning View Details
docs/api/overview.md ✅Succeeded View
docs/api/owner-details-template-resource.md ✅Succeeded View

docs/TOC.md

  • Line 126, Column 6: [Warning: file-not-found - See documentation] Invalid file link: 'api/package-owner-template-resource.md'.

For more details, please refer to the build report.

Note: Your PR may contain errors or warnings or suggestions unrelated to the files you changed. This happens when external dependencies like GitHub alias, Microsoft alias, cross repo links are updated. Please use these instructions to resolve them.

For any questions, please:

Copy link

Learn Build status updates of commit d2d7f17:

✅ Validation status: passed

File Status Preview URL Details
docs/api/overview.md ✅Succeeded View
docs/api/owner-details-template-resource.md ✅Succeeded View
docs/TOC.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Member

@joelverhagen joelverhagen left a comment

Choose a reason for hiding this comment

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

Owner usernames are not asserted to be case insensitive ecosystem wide at this time.

docs/api/owner-details-template-resource.md Outdated Show resolved Hide resolved
## URL template

The URL for the following API is the value of the `@id` property associated with one of the aforementioned
resource `@type` values.
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, but I really don't understand what this sentence means, or what information this "URL template" section is trying to convey.

According to the docs style and voice guide: https://learn.microsoft.com/en-au/contribute/content/style-quick-start
the docs should be written in a casual voice, like a conversation, and use simple, easy to read sentences. I don't think this sentence/paragraph is doing that.

FWIW, I feel like this is a good place for the first example value. Reading the doc top down, it all feels a bit abstract and hard to pin down, until the examples in the last 3 lines of the docs. If there was an example here, early on in the docs page, I think it makes the rest of the docs more relatable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbh I just copied from other documents that have the same section. @joelverhagen I don't see a lot of value in having this section in the documentation, do you think we should keep it? If so, how can we improve this section?

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel strongly. Feel free to edit it according to @zivkan's suggestions. Sounds reasonable.

Copy link

Learn Build status updates of commit 786f6e2:

✅ Validation status: passed

File Status Preview URL Details
docs/api/overview.md ✅Succeeded View
docs/api/owner-details-template-resource.md ✅Succeeded View
docs/TOC.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

Thanks for working on the docs and the template! I think some of the examples are for the package details template.


Name | Type | Required | Notes
----------- | ------- | -------- | -----
`{owner}` | string | no | The owner ID to get details for
Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't considered an optional parameter. Is the idea that every owner link would actually just point to the Gallery?.
This may help package sources to temporarily implement this feature by pointing their owner details hyperlink to some landing page (like Gallery?).
/cc @joelverhagen for additional thoughts

Copy link
Member

Choose a reason for hiding this comment

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

There are two kinds of required that matter I think: 1) the client rejects or errors out when no {owner} parameter is found and 2) we say it's required in the docs because we think it should really be there, but we make no effort to enforce it in the client.

I think having the same URL for every owner (i.e. no {owner} token) is a bad user experience but I don't know how to best surface that error in client.

In short, I think saying required = yes in the docs as a description of intent is okay even if client does no validation on this. required = no gives the wrong intent

Copy link
Contributor

Choose a reason for hiding this comment

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

"Required yes" sounds good to me.

I agree that it's a bad user experience to have identical links for all owners, but we could leave that up to the package source. I wanted to know whether to add validation in Client in my work in progress. I will not and just allow it to get the same repeated URL for each owner.

`{owner}` | string | no | The owner ID to get details for

The casing requirements of {owner} should be defined by the package source.
The client should retain the casing of the username in whatever form it was found in other API responses or in the package repository signature.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Sure, no manipulation of casing on Client


NuGet.org treats owner usernames in a case insensitive manner but not all package source may have this flexibility.

For example, nuget.org's package details template looks like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is talking about package details template, but the example is for an owner details template.

https://www.nuget.org/profile/{owner}
```

If the client implementation needs to display a link to the package details for NuGet.Versioning 4.3.0, it would produce the following URL and provide it to the user:
Copy link
Contributor

Choose a reason for hiding this comment

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

This example appears to be from the package details template.


# Owner URL template

It is possible for a client to build a URL that can be used by the user to see owner's page in their web browser.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
It is possible for a client to build a URL that can be used by the user to see owner's page in their web browser.
It is possible for a client to build a URL that can be used by the user to see a package owner's profile page in their web browser.

# Owner URL template

It is possible for a client to build a URL that can be used by the user to see owner's page in their web browser.
This is useful when a package source wants to show additional information about an owner that may not fit within the scope of what the NuGet client application shows.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure... maybe scratch this for now? As far as I know, nuget.org's owner profile pages are just a profile photo and listings of packages owned by that individual. They don't really add details... or can they?

From my spec's perspective, the purpose is to get some reassurance that the owner is really the owner I expect, by looking at their other published packages on this package source.

Copy link

Learn Build status updates of commit e740e00:

✅ Validation status: passed

File Status Preview URL Details
docs/api/overview.md ✅Succeeded View
docs/api/owner-details-template-resource.md ✅Succeeded View
docs/api/package-details-template-resource.md ✅Succeeded View
docs/TOC.md ✅Succeeded View

For more details, please refer to the build report.

For any questions, please:

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.

5 participants