Clean Up Full CLR Code From Web Cmdlets #5376
Conversation
markekraus
requested review from
dantraMSFT,
JamesWTruher and
PaulHigin
as code owners
|
@markekraus Could you please review all files and add a newline at EOF? |
|
@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 |
|
@markekraus does it make sense to move the files in |
|
@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. |
|
I believe it is more easily and faster to review small PRs. |
|
@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. |
|
@markekraus use |
|
@SteveL-MSFT as an example, I want to take |
|
@markekraus I see, if you're combining files, I agree you should do that as a separate PR. |
|
@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)? |
|
@markekraus I think you can just do 1 PR for combining/move and have them as separate commits |
|
@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
force-pushed
the
WebCmdletsFullClrCleanup
branch
from
e9d8a8f
to
375a3c3
Compare
|
@SteveL-MSFT conflict resolved, but I just realized I never ran Feature tests in this PR. so running now. |
markekraus
force-pushed
the
WebCmdletsFullClrCleanup
branch
from
76edeff
to
3ae8dcd
Compare
|
The one of the fails ( The other is a timeout fail from httpbin.org (I will get those tests all moved over to As for Travis CI, it seems to be having unrelated issues |
|
Yes, we are waiting #5379 to pass CI AppVeyor. |
* rerun CI
markekraus
force-pushed
the
WebCmdletsFullClrCleanup
branch
from
3ae8dcd
to
b6cdbb1
Compare
|
@iSazonov al tests pass now |
closes #5373
reference #4357
reference #3267
#ifdefcode for Full CLRMicrosoft.PowerShell.Commands.Utility.csproj-UseBasicParsingparameter hidden.-UseBasicParsingas the cmdlets now always basic parsing Webcmdlets should parse the <html><head><meta charset="foo"> attribute for the correct encoding if not in http header #3267 (comment)HtmlWebResponseObjectFormsandParsedHtmlfrom format data forBasicHtmlWebResponseObject