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

renames request info to request information #559

Merged
merged 3 commits into from
Aug 31, 2021
Merged

Conversation

baywet
Copy link
Member

@baywet baywet commented Aug 30, 2021

  • renames request info into request information to avoid conflicts
  • bumps patch version

fixes #551

Generation Diff

microsoft/kiota-samples#244

@nikithauc
Copy link
Contributor

@baywet I strongly recommend using Context as a suffix to the class name because:

  1. As I mentioned before in the issue, it would conventional and easier to understand the purpose of the class.
  2. The class in discussion deals with any information used by the middleware. The middleware deals with requests, responses and can be extended by any user to add more features such as session, cache or information dealing with only the responses.

@baywet
Copy link
Member Author

baywet commented Aug 30, 2021

I might be biased here but to me "context" involves some kind of heavy object that's longed lived, but it's probably because of entity framework or the application context in winforms or the app insights telemetry context.
Our request information on the other hand is lightweight and short-lived.
I believe that dotnet devs will feel the same way, arguably TS/JS devs might not.

@MIchaelMainer
Copy link
Member

MIchaelMainer commented Aug 30, 2021

You did describe it as an "...abstract HTTP request". If that description is correct, consider naming to mirror as you described it: AbstractHttpRequest.

For example, does it make sense to authenticate the RequestInformation object, or to authenticate the AbstractHttpRequest object?

@baywet
Copy link
Member Author

baywet commented Aug 30, 2021

Thanks for the suggestion.
Abstract: we're already in the abstraction package, it's redundant.
Http: we're in the abstractions, and would like to stay protocol agnostic

Which leaves us with Request, which is probably going to conflict with a bunch of platform types.
Trust me, I've thought about what to name this class for a while 😂.

Any other suggestion?

@baywet
Copy link
Member Author

baywet commented Aug 30, 2021

Another good candidate I had was RequestDetails, but it's a plural and I'm not sure it'll work for some naming conventions (thinking of java here)

@darrelmiller
Copy link
Member

For better or worse, "context" objects have a pattern of providing both input and output as they contain the context around the operation. The request object should be kept purely as input. It also isn't abstract. Creating an instance of a class that is call abstractXYZ would be counter intuitive.

What's the objection to requestInformation? Having an auth token as part of requestInformation seems fairly natural even though the name is quite long.

@MIchaelMainer
Copy link
Member

The class is active with many methods, so it feels more than informative. It is nearly an HttpRequestMessage or a Fetch Request object, with some additional functionality.

Yeah, we already define RequestContext for use in our middleware. This is different.

I can't come up with another idea that doesn't degrade from RequestInformation or that doesn't start colliding with existing concepts.

MIchaelMainer
MIchaelMainer previously approved these changes Aug 30, 2021
@baywet baywet force-pushed the feature/request-info branch from b269e78 to 63974f1 Compare August 31, 2021 12:30
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

92.9% 92.9% Coverage
0.0% 0.0% Duplication

@baywet baywet merged commit 4f670c3 into main Aug 31, 2021
@baywet baywet deleted the feature/request-info branch August 31, 2021 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Csharp Pull requests that update .net code Go Java Ruby TypeScript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript - Reconsider RequestInfo class name as the name is built-in in the language
4 participants