Skip to content

Refactoring XslCompiledTransform#59475

Merged
krwq merged 3 commits intodotnet:mainfrom
kronic:Refactoring_XslCompiledTransform
Oct 1, 2021
Merged

Refactoring XslCompiledTransform#59475
krwq merged 3 commits intodotnet:mainfrom
kronic:Refactoring_XslCompiledTransform

Conversation

@kronic
Copy link
Contributor

@kronic kronic commented Sep 22, 2021

No description provided.

@ghost ghost added community-contribution Indicates that the PR has been added by a community member area-System.Xml and removed community-contribution Indicates that the PR has been added by a community member labels Sep 22, 2021
@ghost
Copy link

ghost commented Sep 22, 2021

Tagging subscribers to this area: @buyaa-n, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: kronic
Assignees: -
Labels:

area-System.Xml

Milestone: -

Copy link
Contributor

@Wraith2 Wraith2 left a comment

Choose a reason for hiding this comment

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

LGTM in general. As usual I'm not a fan of the unscoped using but that's for the maintainers to decide on.

CheckArguments(input, results);
CheckCommand();
_command.Execute((object)input, documentResolver, arguments, results);
_command!.Execute(input, documentResolver, arguments, results);
Copy link
Member

Choose a reason for hiding this comment

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

why ! hasn't been needed before but it is now?

Reset();
CompileXsltToQil(stylesheet, settings, stylesheetResolver);
return _qil;
return _qil!;
Copy link
Member

Choose a reason for hiding this comment

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

why ! hasn't been needed before and it is now?

Copy link
Member

@krwq krwq left a comment

Choose a reason for hiding this comment

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

LGTM after comments are resolved

@kronic
Copy link
Contributor Author

kronic commented Oct 1, 2021

@krwq I`m removed unused methods. could you run tests?

@krwq
Copy link
Member

krwq commented Oct 1, 2021

/azp list

@krwq
Copy link
Member

krwq commented Oct 1, 2021

/azp run runtime-libraries-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@krwq
Copy link
Member

krwq commented Oct 1, 2021

none of the outerloop test failures look related so LGTM

@krwq krwq merged commit 03e90a5 into dotnet:main Oct 1, 2021
@kronic kronic deleted the Refactoring_XslCompiledTransform branch October 1, 2021 20:13
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

3 participants