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

Michael Klement (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

Ilya (iSazonov) · 2018-12-08T10:32:58Z

+
+                // 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;

}

Ilya (iSazonov) · 2018-12-08T10:34:57Z

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


Only integer?

Ilya (@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.

Ilya (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.

Michael Klement (mklement0) · 2019-01-08T09:50:38Z

Steve Lee (@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.

Andrew (anmenaga)

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

Michael Klement (mklement0) · 2019-01-17T23:26:02Z

Thanks, Andrew (@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?

Ilya (iSazonov) · 2019-01-18T04:51:06Z

Michael Klement (@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

Rain Sallow (/u/ta11ow) (vexx32) · 2019-01-18T05:04:18Z

Michael Klement (@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.

Michael Klement (mklement0) · 2019-01-18T13:23:09Z

I appreciate your help, Rain Sallow (/u/ta11ow) (@vexx32) and Ilya (@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.
Ilya (@iSazonov), what could break?

Ilya (iSazonov) · 2019-01-18T13:25:08Z

After resolving merge conflicts we have to review again.

Ilya (iSazonov) · 2019-01-18T14:07:20Z

Aditya Patwardhan (@adityapatwardhan) The PR is ready to merge.

Michael Klement (mklement0) · 2019-01-18T15:35:17Z

Thanks, Ilya (@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 Rain Sallow (/u/ta11ow) (@vexx32)'s instructions.

Correct?

Ilya (iSazonov) · 2019-01-18T17:08:19Z

Michael Klement (@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.

Aditya Patwardhan (adityapatwardhan) · 2019-01-18T17:56:05Z

Michael Klement (@mklement0) Could you answer Ilya (@iSazonov) suggestion?

Michael Klement (mklement0) · 2019-01-18T18:50:06Z

Aditya Patwardhan (@adityapatwardhan):

Ilya (@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.

Aditya Patwardhan (adityapatwardhan) · 2019-01-18T19:02:45Z

Michael Klement (@mklement0) Thanks for your contribution!

… 'width' entry to be an integer (PowerShell#8426)

Michael Klement (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

Michael Klement (mklement0) requested review from James Truher (JamesWTruher) and Paul Higinbotham (PaulHigin) as code owners December 8, 2018 02:12

Michael Klement (mklement0) added 2 commits December 7, 2018 21:17

CodeFactor style changes

70a830c

Typo

a9b5a9a

Michael Klement (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

Ilya (iSazonov) reviewed Dec 8, 2018

View reviewed changes

Ilya (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

Ilya (iSazonov) requested a review from Mark Kraus (markekraus) December 8, 2018 10:36

Ilya (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

Ilya (iSazonov) requested a review from Steve Lee (SteveL-MSFT) January 8, 2019 10:13

stale Bot removed the Stale label Jan 8, 2019

Andrew (anmenaga) approved these changes Jan 17, 2019

View reviewed changes

Andrew (anmenaga) assigned Aditya Patwardhan (adityapatwardhan) Jan 17, 2019

Michael Klement (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

Aditya Patwardhan (adityapatwardhan) merged commit 20919ee into PowerShell:master Jan 18, 2019

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026

Allow 'name' as an alias key for 'label' in ConvertTo-Html, allow the…

4804083

… 'width' entry to be an integer (PowerShell#8426)

-                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;
+                }

Uh oh!

Conversation

Michael Klement (mklement0) commented Dec 8, 2018

PR Summary

PR Checklist

Uh oh!

Ilya (iSazonov) Dec 8, 2018

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) Dec 8, 2018

Choose a reason for hiding this comment

Uh oh!

Michael Klement (mklement0) Dec 8, 2018

Choose a reason for hiding this comment

Uh oh!

Ilya (iSazonov) left a comment

Choose a reason for hiding this comment

Uh oh!

stale Bot commented Jan 8, 2019

Uh oh!

Michael Klement (mklement0) commented Jan 8, 2019

Uh oh!

Andrew (anmenaga) left a comment

Choose a reason for hiding this comment

Uh oh!

Michael Klement (mklement0) commented Jan 17, 2019

Uh oh!

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

Uh oh!

Uh oh!

Rain Sallow (/u/ta11ow) (vexx32) commented Jan 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Michael Klement (mklement0) commented Jan 18, 2019

Uh oh!

Ilya (iSazonov) commented Jan 18, 2019

Uh oh!

Ilya (iSazonov) commented Jan 18, 2019

Uh oh!

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

Uh oh!

Uh oh!

Ilya (iSazonov) commented Jan 18, 2019

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented Jan 18, 2019

Uh oh!

Michael Klement (mklement0) commented Jan 18, 2019

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ilya (iSazonov) commented Jan 18, 2019 •

edited

Loading

Rain Sallow (/u/ta11ow) (vexx32) commented Jan 18, 2019 •

edited

Loading

Michael Klement (mklement0) commented Jan 18, 2019 •

edited

Loading