-
Notifications
You must be signed in to change notification settings - Fork 113
Conversation
…ookie monster's suggested PowerShell module structure
Exciting! If you need help getting the CI to run the tests just let me know. |
That would be wicked. I don't know a whole lot about AppVeyor but it looks like it's asking for a Visual Studio project. |
Okay I believe I have it down to these tasks to complete:
I think those three are it and all the tests should be passing. |
@Tiberriver256 away from my laptop at the moment but basically add this file: And instead of Does that make sense? |
Specifically, delete this line: And change this to be the right call to run Pester: |
Makes sense. I'll give it a go. |
Looks like it's not attempting to run the build now. |
This reverts commit 61f9b84.
Alright, tests are now passing in PSCore on Windows. I'm installing the WSL Ubuntu to manually test with, if I can figure it out but if you can get the CI build to run again that might be faster. *EDIT: Tests passed on my 16.04 Ubuntu! |
AppVeyor seems to be a bit drunk at the moment and isn't triggering when you push. |
Ohhhh because you're merging into pure-pwsh not master |
The build passed!! 🎉 |
I'll actually review the code and give feedback :) For starters, it looks like the .gitignore and README.md were deleted in this PR, can you add those? |
You can take whatever applies from the old version |
Ah thanks. I put the examples back in as well. Did you want to change the merge to point at the master? |
…and a non-existent HTTP method (405)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WOW. This PR is AWESOME. It's also already super close to being 100% ready to be merged.
I had a few comments on the code and my biggest feedback was some comments in the more complicated bits (which will help future contributors... contribute) and a "nice to have" of having to ability to pass in a Polaris object so that a user could have 2 instances of Polaris running in the same session.
"Nice to have" meaning, if you don't feel like doing it, no worries :) I won't block the PR from getting accepted if you don't want to.
Thank you SO much for doing this. Polaris is really looking great in pure PowerShell! 🎉
Please let me know if you have any comments about my comments. I'm here to help you get this merged in!
MIT License | ||
|
||
Permission is hereby granted, free of charge, to any person obtaining a copy | ||
of this software and associated documentation files (the ""Software""), to deal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this file in the repo. Can you keep this in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be set now
Polaris.psm1
Outdated
{ | ||
Add-Type -Path "$PSScriptRoot/PolarisCore/bin/Debug/net451/Polaris.dll" | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: first line shouldn't be an empty line
Polaris.psm1
Outdated
$script:Polaris = $null | ||
|
||
# Handles the removal of the module | ||
$ExecutionContext.SessionState.Module.OnRemove = | ||
{ | ||
If ( $script:Polaris ) | ||
{ | ||
If ( $script:Polaris ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both Stop-Polaris
and Clear-Polaris
check if $script:Polaris
exists before doing anything so we don't need this check :)
lib/MimeTypes.cs
Outdated
|
||
namespace Polaris | ||
{ | ||
public static class MimeTypes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this file need to C#? This could be just PowerShell, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah... I was being lazy :) It's in PowerShell now and learned an important lesson about PowerShell classes.
C# classes are available globally across runspaces. PowerShell classes are not so they have to be injected into the runspace before execution... blech.
Polaris.psm1
Outdated
$ScriptBlock, | ||
#Get public and private function definition files. | ||
$Classes = @( Get-ChildItem -Path $PSScriptRoot\lib\*.ps1 -ErrorAction SilentlyContinue | where {$_.Name -ne "Polaris.Class.ps1"}) | ||
$Classes += @( Get-ChildItem -Path $PSScriptRoot\lib\Polaris.Class.ps1 -ErrorAction SilentlyContinue ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because we want to load Polaris.Class.ps1
last? If so, can you add a small comment saying that so people know?
Tests/PolarisServer.Tests.ps1
Outdated
$result.Content | Should Be 'test file' | ||
$result.StatusCode | Should Be 200 | ||
} | ||
|
||
It "test /public/index.html static route" { | ||
$expectedHtml = ` | ||
'<div>hello world</div> | ||
'<div>hello world</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this indentation from being on the next line of $expectedHtml
?
|
||
[Int]$Port | ||
[System.Collections.Generic.List[PolarisMiddleWare]]$RouteMiddleWare = [System.Collections.Generic.List[PolarisMiddleWare]]::new() | ||
[System.Collections.Generic.Dictionary[string, [System.Collections.Generic.Dictionary[string, string]]]]$ScriptBlockRoutes = [System.Collections.Generic.Dictionary[string, [System.Collections.Generic.Dictionary[string, string]]]]::new() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this in another comment but now that we're in PowerShell do the Script Blocks have to be stored as a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have to be stored as strings but they eventually would need to be converted to strings in order to be passed to the AddScript method as it only accepts strings.
lib/Polaris.Class.ps1
Outdated
$this.PowerShellPool.Open() | ||
$this.Listener = $this.InitListener($port) | ||
|
||
$syncHash = [hashtable]::Synchronized(@{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments here would be really helpful for future contributors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I'm running short on time here but I pointed towards some blogs that explain how synchronized hashtables work.
lib/Polaris.Class.ps1
Outdated
$response.ByteResponse = $bytes | ||
} | ||
$rawResponse.StatusCode = $response.statusCode; | ||
$rawResponse.Headers = $response.Headers; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use [Polaris]::Send here?
lib/Polaris.Class.ps1
Outdated
$this.Logger($logString) | ||
} | ||
catch { | ||
[Console]::WriteLine($_.Message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for [Console]::WriteLine
instead of Write-Host
or something?
…idation appears to not be aware of PowerShell classes :(
Alrighty, good stuff man! I believe I've addressed all of the requested changes here. Let me know what you think. |
|
||
<# | ||
.SYNOPSIS | ||
Renders a directory browser in HTML |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so excited to try this out :D
There have been a great list of [other](https://github.com/StartAutomating/Pipeworks) [micro](https://github.com/toenuff/flancy) [web](https://github.com/Jaykul/NancyPS/) [frameworks](https://github.com/toenuff/PshOdata) [written](https://github.com/straightdave/presley) [over](https://github.com/cofonseca/WebListener) [the](https://github.com/DataBooster/PS-WebApi) [years](https://github.com/ChristopherGLewis/PowerShellWebServers) (Thanks @jaykul for the list!). | ||
|
||
Polaris' differentiation is that it is cross-platform and uses the .NET HttpListener class. | ||
|
||
## Getting Started | ||
|
||
### Prereqs | ||
* [.NET Standard 2.0 SDK](https://www.microsoft.com/net/download/core) or .NET Framework 4.5.1 | ||
|
||
* [PowerShell](https://github.com/powershell/powershell) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉 Thank you SO much for such an awesome PR
I wanted to offer pulling this into the repo. I might have time to finish it up this weekend but really I think it's so close someone could accept this PR and finish it up pretty quick.
My goal was to do a straight port over to PowerShell without introducing any breaking changes. I wanted to do it by keeping all of the Pester tests the same and just coding PowerShell until I was finished. I broke my rule though and added a static file server and file browser which meant a modification of one of tests.
I'll see if I can run a Pester test tonight and get added to the comments exactly what tests are failing as work items left to be done.
Reference Enhancement Issue #81 for conversation.