Allow 'name' as an alias key for 'label' in ConvertTo-Html, allow the 'width' entry to be an integer by mklement0 · Pull Request #8426 · PowerShell/PowerShell

mklement0 · 2018-12-08T02:12:58Z

PR Summary

Fixes #6994, and additionally allows key width to be specified not just as a [string] (as before), but alternatively as an [int], analogous to the use of width with Format-Table.

PR Checklist

- Allows use of 'name' as an alias for 'label' in calculated properties - Allows specifying a 'width' value as an integer as well as a string

iSazonov · 2018-12-08T10:32:58Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/convert-HTML.cs

+
+                // Accept the width both as a string and as an int.
+                string width;
+                int? widthNum = p.GetEntry(ConvertHTMLParameterDefinitionKeys.WidthEntryKey) as int?;


Why not:

Suggested change

int? widthNum = p.GetEntry(ConvertHTMLParameterDefinitionKeys.WidthEntryKey) as int?;

var widthEntryKey = p.GetEntry(ConvertHTMLParameterDefinitionKeys.WidthEntryKey);

if (widthEntryKey is int widthNum)

{

width = widthNum.Value.ToString();

}

else

{

width = widthEntryKey as string;

}

iSazonov · 2018-12-08T10:34:57Z

test/powershell/Modules/Microsoft.PowerShell.Utility/ConvertTo-Html.Tests.ps1

+        $returnString | Should -Match 'AgeRenamed'
+    }
+
+    It "Test ConvertTo-HTML calculated property supports integer 'width' entry" {


Only integer?

@iSazonov: Good question: int is the type of the Alignment entry for Format-Table, and I simply amended that class to also be able to use it with ConvertTo-Html, with a string-typed width entry.

Since we're talking about UIs and column widths, my guess is that [int] is enough, but I'm not wedded to that, and if someone knows that a wider type is called for, I'm all ears.

iSazonov

LGTM

stale · 2019-01-08T06:26:40Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

mklement0 · 2019-01-08T09:50:38Z

@SteveL-MSFT, what will it take to get this merged? Another review, or just an act of merging by someone who's authorized?

I fear that the related #8427 and #8430 are about to go stale too.

anmenaga

@mklement0 'Codacy/PR Quality Review' needs a minor style update; otherwise this looks fine.

mklement0 · 2019-01-17T23:26:02Z

Thanks, @anmenaga.

I've resolved the code-style issues.

Unfortunately, I haven't been able to resolve the merge conflict, and I'm at a loss as to how to do so.
I've copied the changes that have since been added to the master branch to my branch, yet a conflict is still reported (I've later added a small whitespace fix).

Can you help?

iSazonov · 2019-01-18T04:51:06Z

@mklement0 Please remove/reset last commits (merge resolve) - I'm afraid that you could break something. Then you could use "Resolve conflicts" button in GitHub or ping me for help.

# Reset to commit "ooooooo"
git reset --hard ooooooo  
# Remove last commit
git reset --hard HEAD~1

vexx32 · 2019-01-18T05:04:18Z

@mklement0 Did you make a merge commit or just copy the changes across?

If the latter, make sure your master branch is up to date, change to this branch, and then git merge master. Resolve conflicts in your editor and make the final merge commit, then push the changes. That will usually resolve the conflict.

Also note that if you need to reset back from some commits, as Ilya mentions you may want to do, you will need a force-push to remove them from the remote once you're done.

mklement0 · 2019-01-18T13:23:09Z

I appreciate your help, @vexx32 and @iSazonov.

Indeed I hadn't merged locally and had just tried to copy the changes manually.
(Merging on GitHub wasn't possible.)

I've now merged locally and pushed again, which resolves the conflict.

Do I still need to reset something?
Seems like you could now simply squash-merge.
@iSazonov, what could break?

iSazonov · 2019-01-18T13:25:08Z

After resolving merge conflicts we have to review again.

iSazonov · 2019-01-18T14:07:20Z

@adityapatwardhan The PR is ready to merge.

mklement0 · 2019-01-18T15:35:17Z

Thanks, @iSazonov.

Just so I get it right in the future, the procedure should be:

Use GitHub's own merging feature if available (it wasn't in this case).
Otherwise, merge locally first from an up-to-date master branch, then push, as per @vexx32's instructions.

Correct?

iSazonov · 2019-01-18T17:08:19Z

@mklement0 Currently GitHub "resolve merge conflicts" feature works very well and can be used for fast resolving simple conflicts. Large and complex conflicts we have to do locally. I guess there is many ways to do this in git. You can use a way you prefer. Usually I use git pull --rebase PowerShell master then follow git hints.

adityapatwardhan · 2019-01-18T17:56:05Z

@mklement0 Could you answer @iSazonov suggestion?

mklement0 · 2019-01-18T18:50:06Z

@adityapatwardhan:

@iSazonov was responding how to handle this situation in the future - duly noted by me (and acknowledged with a thumbs-up).

Is there something left to resolve for this PR?
My (superficial) understanding is that the way I've eventually resolved the merge conflict should now allow for squash-merging.

adityapatwardhan · 2019-01-18T19:02:45Z

@mklement0 Thanks for your contribution!

mklement0 added 2 commits December 7, 2018 21:02

Fixes PowerShell#6994

f0e5ada

- Allows use of 'name' as an alias for 'label' in calculated properties - Allows specifying a 'width' value as an integer as well as a string

Reverted accidental change to launch.json

12c24f3

mklement0 requested review from JamesWTruher and PaulHigin as code owners December 8, 2018 02:12

mklement0 added 2 commits December 7, 2018 21:17

CodeFactor style changes

70a830c

Typo

a9b5a9a

mklement0 changed the title ~~Allow 'name' as an alias key for 'label' in ConvertTo-Html, allow the 'width' value to be an integer~~ Allow 'name' as an alias key for 'label' in ConvertTo-Html, allow the 'width' entry to be an integer Dec 8, 2018

switched to using LabelEntryDefinion() instance

8e5cb3a

iSazonov reviewed Dec 8, 2018

View reviewed changes

iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Dec 8, 2018

iSazonov requested a review from markekraus December 8, 2018 10:36

iSazonov approved these changes Dec 8, 2018

View reviewed changes

This was referenced Dec 8, 2018

Consider creating a conceptual help topic for calculated properties MicrosoftDocs/PowerShell-Docs#3383

Closed

[Feature] Define short alias names for all keys supported in calculated-property definitions #8430

Closed

stale bot added the Stale label Jan 8, 2019

iSazonov requested a review from SteveL-MSFT January 8, 2019 10:13

stale bot removed the Stale label Jan 8, 2019

anmenaga approved these changes Jan 17, 2019

View reviewed changes

anmenaga assigned adityapatwardhan Jan 17, 2019

mklement0 added 7 commits January 17, 2019 16:17

addresses Codacy/PR Quality Review issue

144bd99

addresses CodeFactor issues

db864f1

resolves merge conflict

3e1b38a

resolves merge conflict

b993fad

???

529fb98

resolves merge conflict

d3a6975

resolves merge conflict

70614e2

resolves merge conflict

10c064c

adityapatwardhan merged commit 20919ee into PowerShell:master Jan 18, 2019

- int? widthNum = p.GetEntry(ConvertHTMLParameterDefinitionKeys.WidthEntryKey) as int?;
+ var widthEntryKey = p.GetEntry(ConvertHTMLParameterDefinitionKeys.WidthEntryKey);
+ if (widthEntryKey is int widthNum)
+ {
+ width = widthNum.Value.ToString();
+ }
+                else
+ {
+ width = widthEntryKey as string;
+ }

Conversation

mklement0 commented Dec 8, 2018

PR Summary

PR Checklist

Uh oh!

iSazonov Dec 8, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov Dec 8, 2018

Choose a reason for hiding this comment

Uh oh!

mklement0 Dec 8, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jan 8, 2019

Uh oh!

mklement0 commented Jan 8, 2019

Uh oh!

anmenaga left a comment

Choose a reason for hiding this comment

Uh oh!

mklement0 commented Jan 17, 2019

Uh oh!

iSazonov commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vexx32 commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mklement0 commented Jan 18, 2019

Uh oh!

iSazonov commented Jan 18, 2019

Uh oh!

iSazonov commented Jan 18, 2019

Uh oh!

mklement0 commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Jan 18, 2019

Uh oh!

adityapatwardhan commented Jan 18, 2019

Uh oh!

mklement0 commented Jan 18, 2019

Uh oh!

adityapatwardhan commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

iSazonov commented Jan 18, 2019 •

edited

Loading

vexx32 commented Jan 18, 2019 •

edited

Loading

mklement0 commented Jan 18, 2019 •

edited

Loading