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

docs(dotnet): reference the available constants for csharp #5785

Merged
merged 1 commit into from
Mar 18, 2021
Merged

docs(dotnet): reference the available constants for csharp #5785

merged 1 commit into from
Mar 18, 2021

Conversation

avodovnik
Copy link
Contributor

Since the resourceType returned is strings, C# introduces some constants for this, to make comparison easier. This change makes them discoverable for the user.

@avodovnik avodovnik requested a review from yury-s March 10, 2021 18:19
@avodovnik avodovnik changed the title chore(docs): reference the available constants for csharp docs(dotnet): reference the available constants for csharp Mar 10, 2021
@@ -157,6 +157,13 @@ Contains the request's resource type as it was perceived by the rendering engine
following: `document`, `stylesheet`, `image`, `media`, `font`, `script`, `texttrack`, `xhr`, `fetch`, `eventsource`,
`websocket`, `manifest`, `other`.

## method: Request.resourceType
* langs: csharp
Copy link
Member

Choose a reason for hiding this comment

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

This will only override return type but not the comment of the method. To override both you need to either explicitly list other langs in the original definition or update our docs parser.

Returned value is a string everywhere else (including typescript where we could easily narrow it down to a list of constants) and nobody complained so far. I'd rather we kept C# API consistent with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, looks like I discarded one change too many before pushing this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I did only intend to override the comment, not the actual return type. We still return string (to keep things consistent), but the strings are available in the C# port through the constants file. What did you do for these cases in Java to help discoverability?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I did only intend to override the comment, not the actual return type.

I don't think it's supported by current parser, it's either return type override or full method (if the lang filters are mutually exclusive).

What did you do for these cases in Java to help discoverability?

Nothing for now (we don't provide such assistance in any other language), just the documentation. It was tempting to create constants or even enum in java too but we decided to keep it simple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the override worked, but I thought maybe just having a small, language-specific, note would help, so here's a PR that would render this PR useless: #5810

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming #5810 merges, I've made this change "smaller" and using that feature :-).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: when I merge #5810, I'll rebase this, and the build error here should go away

@avodovnik avodovnik requested a review from yury-s March 17, 2021 17:33
@avodovnik avodovnik merged commit dfb1c99 into microsoft:master Mar 18, 2021
@avodovnik avodovnik deleted the dev/anvod/docs/requestresponse branch March 18, 2021 15:34
pavelfeldman added a commit that referenced this pull request Mar 18, 2021
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.

2 participants