Skip to content

Generated v1.0 models and request builders using Typewriter#812

Merged
MIchaelMainer merged 1 commit into
devfrom
v1.0/pipelinebuild/3825480
Oct 9, 2020
Merged

Generated v1.0 models and request builders using Typewriter#812
MIchaelMainer merged 1 commit into
devfrom
v1.0/pipelinebuild/3825480

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot commented Oct 8, 2020

This pull request was automatically created by the GitHub Action, create pull request.

The commit hash is 9957d57.

Important Check for unexpected deletions or changes in this PR.

  • Check that the changes match the changes in the captured commit for v1.0_metadata.xml

cc: @darrelmiller

Comment thread typeSummary.txt
Comment on lines +17602 to +17605
class Microsoft.Graph.CallRecords.Endpoint
property AdditionalData : System.Collections.Generic.IDictionary`2[[System.String],[System.Object]]
property ODataType : System.String
property UserAgent : Microsoft.Graph.CallRecords.UserAgent
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: note to self (@zengin). why is the sort order not canonical?

Comment thread typeSummary.txt
Comment on lines -48259 to +48392
return type System.Threading.Tasks.Task`1[[System.Boolean]]
return type System.Threading.Tasks.Task`1[[System.Nullable`1[[System.Boolean]]]]
Copy link
Copy Markdown
Contributor

@zengin zengin Oct 8, 2020

Choose a reason for hiding this comment

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

@MIchaelMainer
Sorry for not catching this earlier, but this is a breaking change. Unfortunately this is already in but is reflected in type summary today because previous check-in did not generate the type summary, i.e. the commit was not generated using the pipeline.

See the linqpad example below for how people's code can break:

void Main()
{
	var output = GetAsync().Result;
	var output2 = GetAsync2().Result;
	if (output)
	{
		Console.WriteLine("Compiles");
	}
	
	if (output2)
	{
		Console.WriteLine("Does not compile because output2 is a nullable boolean and can't be used directly in if conditions");
	}
}

public async Task<bool> GetAsync()
{
	return true;
}

public async Task<bool?> GetAsync2()
{
	return true;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@zengin You are technically correct. The thing is that it was already broken and wasn't working before. So no one could've taken a dependency on these features. So this change does make it not breaking. The issue for me is that how did this not get in the manual build?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification on the breaking change.

Did you generate #800 using the pipeline? If not, it is expected that the type summary was not updated.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No, I didn't. It would've be useful for me to capture summary in my PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@baywet @nikithauc @MIchaelMainer

Btw, I run into these non-breaking breaking changes every now and then. Can we come up (or reuse) some term for this so that I can just say bla? and bla? represents:

This is technically a breaking change, but is this one of those cases where nobody could've taken a dependency on this due to broken state so it is not a breaking change?

Does anybody know such a term?
cc: @ddyett @darrelmiller @peombwa

Copy link
Copy Markdown
Contributor

@zengin zengin left a comment

Choose a reason for hiding this comment

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

I am confused about the removal of MessageRequestBuilder->Forward.

The metadata seems to be only moving message->forward, but adding event->forward. This doesn't match with the change here.

@zengin
Copy link
Copy Markdown
Contributor

zengin commented Oct 8, 2020

I am confused about the removal of MessageRequestBuilder->Forward.

The metadata seems to be only moving message->forward, but adding event->forward. This doesn't match with the change here.

This is resolved. It is only moved.

@MIchaelMainer MIchaelMainer merged commit 40b6a0b into dev Oct 9, 2020
@MIchaelMainer MIchaelMainer deleted the v1.0/pipelinebuild/3825480 branch October 9, 2020 21:07
@MIchaelMainer MIchaelMainer mentioned this pull request Oct 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants