Skip to content

Proposed fix for #473 #474

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

Merged
merged 4 commits into from
Mar 29, 2016
Merged

Proposed fix for #473 #474

merged 4 commits into from
Mar 29, 2016

Conversation

kilasuit
Copy link
Contributor

This adds the Capitalised versions of the Function & Variable scopes though this could perhaps be done much cleaner than in this way


This change is Reviewable

@KirkMunro
Copy link
Contributor

That's not the right fix for this issue, because you could have scope identifiers with all caps, mixed upper and lowercase, etc. The comparison/lookup logic is simply doing a case sensitive lookup, and it should change to do a case insensitive lookup instead.

@kilasuit
Copy link
Contributor Author

@KirkMunro - I agree that the logic should be case insensitive however this is outside of my experience with C# - hence the "dirty fix"

@KirkMunro
Copy link
Contributor

I wouldn't bother proposing dirty fixes in that case, because you're just
creating more work by submitting a pull request that needs to be rejected.
Better to simply comment that the issue seems to be that the current logic
does a case sensitive comparison with the scope label when it should be
doing a case insensitive comparison and then leave it for someone who can
do the fix properly.

*Kirk Munro *
Poshoholic, Microsoft MVP
Blog http://poshoholic.com/ | Twitter http://twitter.com/poshoholic |
LinkedIn http://ca.linkedin.com/in/kirkmunro | GitHub
http://github.com/KirkMunro | Facebook
http://www.facebook.com/#%21/kirk.munro

Need a PowerShell SME for a project at work? Contact me
http://poshoholic.com/contact-me/!

On Tue, Mar 22, 2016 at 11:40 AM, Ryan Yates [email protected]
wrote:

@KirkMunro https://github.com/KirkMunro - I agree that the logic should
be case insensitive however this is outside of my experience with C# -
hence the "dirty fix"


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#474 (comment)

@kilasuit
Copy link
Contributor Author

I'm going to attempt to get this fixed properly by the end of the weekend with additional tests in place to cover additional scenarios suggested by @KirkMunro

I think I'm almost there with it.

@kilasuit
Copy link
Contributor Author

Now updating the tests for this

@kilasuit kilasuit changed the title Proposed dirty fix for #473 Proposed fix for #473 Mar 24, 2016
@kapilmb
Copy link

kapilmb commented Mar 29, 2016

Thanks for the fix.


Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@kapilmb kapilmb merged commit a8ea91b into PowerShell:development Mar 29, 2016
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.

4 participants