Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from
Dec 7, 2017

Conversation

TravisEz13
Copy link
Member

@TravisEz13 TravisEz13 commented Dec 4, 2017

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
Member Author

restarted appveyor due to web timeout

Copy link
Member

@adityapatwardhan adityapatwardhan left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments.

}
}
# Verify depenecies are installed and in the path
Test-Dependecies
Copy link
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
Member Author

Choose a reason for hiding this comment

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

resolved

Start-NativeExecution { gzip -f $RoffFile }

$ManFile = Join-Path "/usr/local/share/man/man1" (Split-Path -Leaf $GzipFile)
# Generate gzip of mann file
Copy link
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
Member Author

Choose a reason for hiding this comment

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

resolved

[String]$Description,

[Parameter(Mandatory,HelpMessage='Installer Type')]
[String]$Type,
Copy link
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
Member Author

Choose a reason for hiding this comment

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

resolved

[Parameter(Mandatory,HelpMessage='The built and gzipped mann file.')]
[String]$MannGzipFile,

[Parameter(Mandatory,HelpMessage='The destination of the mann file')]
Copy link
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
Member Author

Choose a reason for hiding this comment

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

resolved

[String]$AfterRemoveScript
)

$Arguments = @(
Copy link
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
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


if ( ($Environment.IsUbuntu -or $Environment.IsDebian) -and !$Distribution )
{
throw "$Distribution is require for a Debian based distribution."
Copy link
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
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
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
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
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
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.

None yet

2 participants