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

New-VisioRackShape v1.1 PR #78

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mc1903
Copy link

@mc1903 mc1903 commented Sep 26, 2021

Hi @MikeShepard,

Pull Request for adding the New-VisioRackShape function as discussed in #77

I have included an example in the CBH and an extended example in the Examples folder.

Let me know if you want any changes/etc.

Cheers.
M

Updated VisioShape.ps1 to add the new function New-VisioRackShape 
Updated VisioBot3000.psd1 FunctionsToExport to include New-VisioRackShape
Adds New-VisioRackShape_Example.ps1 to \Examples
@MikeShepard MikeShepard marked this pull request as draft September 27, 2021 13:23
@MikeShepard MikeShepard self-requested a review September 27, 2021 13:23
Copy link
Owner

@MikeShepard MikeShepard left a comment

Choose a reason for hiding this comment

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

Overall really clean. A few comments/questions/suggestions in the main function

VisioShape.ps1 Outdated Show resolved Hide resolved
VisioShape.ps1 Outdated
.PARAMETER FirstU
The first (lowest) U space in which to place the new rack equipment shape.

.PARAMETER X
Copy link
Owner

Choose a reason for hiding this comment

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

confused by the x and y parameters. You're going to move the shape automatically, so why not just use these default values and not have the user supply them?

Copy link
Author

Choose a reason for hiding this comment

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

I decided it would be nice to allow the user to chose where the initial shape is dropped prior to connecting it to the rack shape. Parameters $X and $Y are not mandatory. If they are not supplied they default to $X=1.00 and $Y=0.25.

Copy link
Owner

Choose a reason for hiding this comment

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

If you want to have default values, put that in the parameter definition...
[Parameter(Mandatory=$false)]$X=1.00

Copy link
Author

Choose a reason for hiding this comment

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

If you want to have default values, put that in the parameter definition... [Parameter(Mandatory=$false)]$X=1.00

I genuinely didn't know you could do that #everydayisaschoolday :-)

I will amend the $X and $Y defaults when I have some free time later in the week/weekend. Thx

VisioShape.ps1 Outdated
}

<#
@Mike - unsure where $updatemode is set and if I need to worry about it here?
Copy link
Owner

Choose a reason for hiding this comment

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

So, UpdateMode is kind of unsupported and experimental (and cool).

I would probably use an if/then like this:
if($updateMode){
$droppedshape=... (as in the commented code)
} else {
$droppedShape=$p.Drop .... (as you have it)
}

Copy link
Author

Choose a reason for hiding this comment

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

Updated as per feedback in VisioShape.ps1 - mc1903@b94cc8f

Copy link
Owner

Choose a reason for hiding this comment

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

I should have been more clear.

In UpdateMode, to point is to use shapes on the page if they exist...if not, create a shape as necessary.

If you look at new-visioshape you can see how this might look.

As is, you wouldn't update the position of a rack shape in update mode.

Probably not a huge deal, but if we can make it work it would be better.

Copy link
Author

Choose a reason for hiding this comment

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

I should have been more clear...

No, I am probably being a bit thick :-).

I assume I just set $UpdateMode=$true before calling New-VisioRackShape with updated rack equipment shape BeginXY and EndXY placements?

Again, I will look to fix this later in the week/weekend. Thx.

Copy link
Author

Choose a reason for hiding this comment

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

Hi Mike,

I hope you are well. Sorry it's taken me longer than I had planned to look at this again. It got busy at work and I spent most of last weekend replacing a built-in dishwasher; which turned out to be crash course in electrics, plumbing and carpentry!

I am still not getting how I invoke UpdateMode in order to test it. If you have 5 mins can you give me an example of how it should work/be invoked? I did look at New-VisioShape as you suggested but it doesn't help me tbh.

Do I need to add a -UpdateMode parameter to the New-VisioRackShape function? If this is the case, would a Set-VisioRackShape function be better for moving existing shapes?

Cheers
M

@MikeShepard MikeShepard marked this pull request as ready for review September 27, 2021 13:49
Updated New-VisioRackShape function (now v1.2) based on Mike's initial PR78 feedback. MikeShepard#78 (comment)
@mc1903 mc1903 requested a review from MikeShepard October 1, 2021 09:01
@MikeShepard
Copy link
Owner

MikeShepard commented Oct 16, 2021 via email

@MikeShepard
Copy link
Owner

MikeShepard commented Oct 22, 2021 via email

@MikeShepard
Copy link
Owner

Finally worked up an update example.

After running your rack example and saving the doc in c:\temp\rack.vsdx, move the rack to the middle of the page (manually) and save the diagram.

Copy your example script and change the new-visiodocument to open-VisioDocument -path c:\temp\rack1.vsdx -Update

This will open the document in "update mode"

Then, add to the end of the script this line (which just adds a new server to the rack)
New-VisioRackShape -Master HPEDL360Gen108SFF -Label Server6 -RackLabel Rack01 -RackVendor HPE -FirstU 13

If it works, it would not move the rack back to the position specified in the script, but it would add the new server to the rack.

I've adjusted the VisioShape.ps1 file and attached it here. I think I got it working.
VisioShape.zip

Just noticed that you're outputting $droppedShape if Verbose is specified...not sure why you'd to that.

@mc1903
Copy link
Author

mc1903 commented Nov 4, 2021

Hi Mike,

Excellent, thank you. I will take a look over the weekend.

Ref "Just noticed that you're outputting $droppedShape if Verbose is specified...not sure why you'd to that."

As I was adding lots of rack equipment shapes, one after the other, the pages of $droppedShape output were just flashing past and giving me a bit of a headache. I felt I would only want to see the $droppedShape output if something wasn't working and I ran the function with -Verbose.

Cheers
M

@MikeShepard
Copy link
Owner

MikeShepard commented Nov 4, 2021 via email

Amended New-VisioRackShape function (now v1.4) based on Mike's feedback regarding UpdateMode.
@mc1903
Copy link
Author

mc1903 commented Nov 14, 2021

Morning Mike.

Updated as v1.4.

I encountered this issue with your suggested logic for $UpdateMode, so made a few changes that work for the following scenarios:

  1. Normal Mode; Add New Shape
  2. Update Mode; Move Existing Shape
  3. Update Mode; Add New Shape
PS C:\Users\Martin> New-VisioRackShape -Master HPEDL180Gen108LFF -Label Server1 -RackLabel Rack01 -RackVendor HPE -FirstU 1
The variable '$DroppedShape' cannot be retrieved because it has not been set.
At C:\Program Files\WindowsPowerShell\Modules\VisioBot3000\1.1\VisioShape.ps1:274 char:18
+         If (-not $DroppedShape){
+                  ~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (DroppedShape:String) [], RuntimeException
    + FullyQualifiedErrorId : VariableIsUndefined 

Also, I took your earlier comments onboard, regarding my previous change to allow specifying the initial $X and $Y drop position, and have gone back to a fixed 0,0 position. But to mask the shape transition between the initial drop position and the final rack U position, I am disabling screen updating, dropping/moving and the enabling screen updating.

I am not too familiar with DSL, so have removed the logic that was suppressing the $DroppedShape output.

I hope this works for you, please let me know what you think.

Cheers
M

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