Skip to content

Clean Up Full CLR Code From Web Cmdlets - #5376

Merged
Ilya (iSazonov) merged 2 commits into
PowerShell:masterfrom
markekraus:WebCmdletsFullClrCleanup
Nov 10, 2017
Merged

Ilya (iSazonov) merged 2 commits into
PowerShell:masterfrom
markekraus:WebCmdletsFullClrCleanup

Conversation

@markekraus

Copy link
Copy Markdown
Contributor

closes #5373
reference #4357
reference #3267

@iSazonov

Copy link
Copy Markdown
Collaborator

Mark Kraus (@markekraus) Could you please review all files and add a newline at EOF?

@markekraus

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) All of the files I modified in the PR have a newline EOF. Some of them did not have one before and just ended with #endif.

@SteveL-MSFT

Copy link
Copy Markdown
Member

Mark Kraus (@markekraus) does it make sense to move the files in common and CoreCLR to the parent and get rid of those subdirs?

@markekraus

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) If it were up to me. I would combine the core partials and the common partials and move them all to the parent webcmdlet folder. It would definitely make these a lot easier to work on since there is no FullCLR to contend with. I'm not sure if it makes sense to do that and this cleanup in the same PR, though. But I would definitely want it that way, whether in this PR or another.

@iSazonov

Copy link
Copy Markdown
Collaborator

I believe it is more easily and faster to review small PRs.
The PR LGTM and I vote to merge now.

@markekraus

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) do you think I should do the merging of partials in one PR and then the file moves in another? otherwise... it will just look like I created new classes in parent folder and deleted old ones. Hard to see how they were put together. I could just do them as 2 commits in the same PR too.

@SteveL-MSFT

Copy link
Copy Markdown
Member

Mark Kraus (@markekraus) use git mv and git will report the file was just moved

@markekraus

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) as an example, I want to take WebCmdlet\Common\BasicHtmlWebResponseObject.Common.cs and WebCmdlet\CoreCLR\BasicHtmlWebResponseObject.CoreClr.cs, and combine them to WebCmdlet\BasicHtmlWebResponseObject.cs

@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Nov 8, 2017

Copy link
Copy Markdown
Member

Mark Kraus (@markekraus) I see, if you're combining files, I agree you should do that as a separate PR.

@markekraus

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) for clarity, should I do 1 PR for combining, and another PR for the move? Or just do both in 1 PR (with 2 commits)?

@SteveL-MSFT

Copy link
Copy Markdown
Member

Mark Kraus (@markekraus) I think you can just do 1 PR for combining/move and have them as separate commits

@SteveL-MSFT

Copy link
Copy Markdown
Member

Mark Kraus (@markekraus) can you resolve the merge conflict? As discussed in the issue, we'll defer further clean-up work and should merge this once the merge conflict is resolved

@markekraus

Copy link
Copy Markdown
Contributor Author

Steve Lee (@SteveL-MSFT) conflict resolved, but I just realized I never ran Feature tests in this PR. so running now.

@markekraus

Copy link
Copy Markdown
Contributor Author

The one of the fails (EventResource) in AppVeyor is appearing in all builds, not just this one. I assume it's a known issue.

The other is a timeout fail from httpbin.org (I will get those tests all moved over to WebListener someday).

As for Travis CI, it seems to be having unrelated issues

@iSazonov

Copy link
Copy Markdown
Collaborator

Yes, we are waiting #5379 to pass CI AppVeyor.

@markekraus

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) al tests pass now

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.

Format data for BasicHtmlWebResponseObject contains ParsedHtml and Forms properties

3 participants