Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Packaging: Try to make New-Unix package more readable#5625

Merged
adityapatwardhan merged 9 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
TravisEz13:macOsPackagingTravisEz13/PowerShell:macOsPackagingCopy head branch name to clipboard
Dec 7, 2017
Merged

Packaging: Try to make New-Unix package more readable#5625
adityapatwardhan merged 9 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
TravisEz13:macOsPackagingTravisEz13/PowerShell:macOsPackagingCopy head branch name to clipboard

Conversation

@TravisEz13

@TravisEz13 TravisEz13 commented Dec 4, 2017

Copy link
Copy Markdown
Member

PR Checklist

Note: Please mark anything not applicable to this PR NA.

PR Summary

Move large chunks of code with-in a single function out into functions to make the function easier to understand.

@TravisEz13 TravisEz13 force-pushed the macOsPackaging branch 2 times, most recently from a5071a0 to 0383540 Compare December 4, 2017 23:01
@TravisEz13 TravisEz13 changed the title Packaging: Try to make New-Unix package more readable [Don't Merge] Packaging: Try to make New-Unix package more readable Dec 4, 2017
@TravisEz13 TravisEz13 force-pushed the macOsPackaging branch 2 times, most recently from 2b7e98f to 093ab75 Compare December 5, 2017 01:38
@TravisEz13 TravisEz13 changed the title [Don't Merge] Packaging: Try to make New-Unix package more readable Packaging: Try to make New-Unix package more readable Dec 5, 2017
@TravisEz13

Copy link
Copy Markdown
Member Author

restarted appveyor due to web timeout

@adityapatwardhan adityapatwardhan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with minor comments.

Comment thread tools/packaging/packaging.psm1 Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typo in dependencies, also in the comment above.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

resolved

Comment thread tools/packaging/packaging.psm1 Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

mann -> man

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

resolved

Comment thread tools/packaging/packaging.psm1 Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please add a comment for the possibilities.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

resolved

Comment thread tools/packaging/packaging.psm1 Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Mann -> Man. Also line 770

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

resolved

Comment thread tools/packaging/packaging.psm1 Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Consider ArrayList

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This became unreadable after changing to an array list. I filed this issue to add a feature to PowerShell so I can maintain a similar syntax and use a list:
#5643

Comment thread tools/packaging/packaging.psm1 Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

require -> required

throw "Dependency precheck failed!"
}
}
# Verify depenecies are installed and in the path

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Typo in comment.

if ($AfterInstallScript) {
Remove-Item -erroraction 'silentlycontinue' $AfterInstallScript
if ($AfterScriptInfo.AfterInstallScript) {
Remove-Item -erroraction 'silentlycontinue' $AfterScriptInfo.AfterInstallScript

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-Force maybe?

if ($AfterRemoveScript) {
Remove-Item -erroraction 'silentlycontinue' $AfterRemoveScript
if ($AfterScriptInfo.AfterRemoveScript) {
Remove-Item -erroraction 'silentlycontinue' $AfterScriptInfo.AfterRemoveScript

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

-Force maybe?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There will be a follow-up PR. I can address these issue in that PR.

@adityapatwardhan

Copy link
Copy Markdown
Member

According to @TravisEz13 the remaining changes will be done in the next PR.

@adityapatwardhan adityapatwardhan merged commit c367a9d into PowerShell:master Dec 7, 2017
@TravisEz13 TravisEz13 deleted the macOsPackaging branch December 7, 2017 18:58
TravisEz13 added a commit to TravisEz13/PowerShell that referenced this pull request Dec 7, 2017
@TravisEz13 TravisEz13 added this to the 6.0.0-RC.2 milestone Dec 8, 2017
TravisEz13 added a commit to TravisEz13/PowerShell that referenced this pull request Dec 8, 2017
* refactor start-pspackage into functions

* [Package] Added instrumentation

* [Package] update change log

* [Package] Fix distribution parameter in get-dependecies

* [Package] fix dependencies

* [Package] fix issues with validate script
TravisEz13 added a commit that referenced this pull request Dec 8, 2017
* refactor start-pspackage into functions

* [Package] Added instrumentation

* [Package] update change log

* [Package] Fix distribution parameter in get-dependecies

* [Package] fix dependencies

* [Package] fix issues with validate script
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.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.