adds constructors to request builders #322
Conversation
baywet
requested review from
abfarah,
darrelmiller,
ddyett,
MIchaelMainer,
nikithauc,
peombwa and
zengin
as code owners
baywet
force-pushed
the
feature/request-builders-ctors
branch
from
1cfb31c to
2759fe9
Compare
baywet
force-pushed
the
feature/client-configuration
branch
from
3974e6f to
82d43c4
Compare
baywet
force-pushed
the
feature/request-builders-ctors
branch
from
2759fe9 to
5267043
Compare
There was a problem hiding this comment.
Since we aren't using templates (not that we used them correctly in typewriter), I want to stress how useful it would be to comment each method with example code it is expected to write. it gives the code reader a reference point to understand the code before reading the code.
| return codeClass; | ||
| } | ||
|
|
||
| private static readonly string httpCoreParameterName = "httpCore"; |
There was a problem hiding this comment.
Consider making coreInterfaceType value configurable as not all languages use the I* interface naming convention. For example, PHP convention would make this HttpCoreInterface.
https://www.php-fig.org/bylaws/psr-naming-conventions/
There was a problem hiding this comment.
those kind of corrections are handled at the refiner level. That's because I want to contain any language specific work to the refiners and writers to avoid a spread (and the maintenance burden that comes with it). see
| ParameterKind = CodeParameterKind.CurrentPath, | ||
| }); | ||
| } | ||
| constructor.AddParameter(new CodeParameter(constructor) { |
There was a problem hiding this comment.
Global comment
Please add comments. Reading code that writes code is not straightforward. You are very intimate with what the code does, everyone else is not. Bonus if you add inline examples of what code will be created by this code. Future maintainers (and code reviewers) will appreciate your empathy.
There was a problem hiding this comment.
I won't add samples as comments in the code, this would get messy really quick, this is why we have the samples repo and why I always join an update whenever things change. Unit tests should also help decipher the code (which I though was self-explanatory).
I can however add doc comments on things like enum values (methodkin, parameterkind...) to clarify the intent of each specialized version of a code element. Thoughts?
Additionally, the doc comments added with #324 should help clarify a lot of the expectations (although that was intended for library implementers, it'll also help people working on other languages)
| @@ -84,6 +87,17 @@ public override void WriteCodeElement(CodeMethod codeElement, LanguageWriter wri | |||
| .ThenBy(x => x.Name)) { | |||
| writer.WriteLine($"{propWithDefault.Name.ToFirstCharacterUpperCase()} = {propWithDefault.DefaultValue};"); | |||
| } | |||
| if(currentMethod.IsOfKind(CodeMethodKind.Constructor)) { | |||
There was a problem hiding this comment.
The code here seems impacted by the code in https://github.com/microsoft/kiota/pull/322/files#diff-61c9c35ebb5b7e20af529c42cc88f8849f5c4df4e585595b8e7964cee57b343dR336. Consider adding a test to make sure a change in KiotaBuilder is paired with a change here. Or consider a comment in both places to help indicate that the code here is related to the code in KiotaBuilder.
This comment is applicable to all /writers/**/CodeMethodWriter.cs code files.
There was a problem hiding this comment.
When we have a look at the design overview it's clear that writers and refiners, which are downstream of the builder, will be impacted by changes made in the builder. The only "hole" I can see in the docs is it doesn't callout the components names specifically.
Again, the builder's responsibility is to paint the most accurate picture of "here is all the things we should represent in the outcome", refiners apply a "lens" on it to taint it according to the target language. The writers may choose or not to implement specific details. While I'm making all the work on a single front for the 3 prime languages so we can make rapid progress and get early feedback, I'm not expecting the writers to implement every detail from day one, or at all depending on the language's philosophy. I'm not expecting a lot of changes to happen in the builder either after the ground work is done, the action is going to happen in the refiners/writers/libraries for each language.
All that being said, I recognize the code method writers are becoming "hairy", I've contemplated for a while splitting those and introducing new code elements to make distinctions and maintenance easier. Things like "CodeConstructor" and "CodeAccessor". This should lead to dividing up the number of lines in the method writers by 2/3 but might lead to a little duplication as well. Do you have any thoughts on that after going through multiple reviews?
baywet
force-pushed
the
feature/request-builders-ctors
branch
from
5267043 to
8f4b91a
Compare
baywet
force-pushed
the
feature/request-builders-ctors
branch
from
c7acaac to
276e900
Compare
baywet
added
the
generator
There was a problem hiding this comment.
The samples submodule seems to point to a different commit than the head of the microsoft/kiota-samples#135
Apart from the couple of comments this looks good to me.
|
Kudos, SonarCloud Quality Gate passed! |
Summary
fixes #301
This pull request adds constructors to request builders so they:
Generetion changes
microsoft/kiota-samples#135