search

aspnet / dnvm

OBSOLETE - see readme
Other
174 stars 61 forks source link

-r, -a and -OS parameters are now case insensitive #469

Closed bjorkstromm closed 8 years ago

bjorkstromm commented 8 years ago

__dnvm_requested_version_or_alias parameters are now case insensitive and dnvm alias can now take 9 parameters.

This fixes #449 and also other issues caused by case sensitivity in commands using __dnvm_requested_version_or_alias

Was this also an issue on Windows? Not currently near a Windows machine so I can't verify.

//cc @muratg @glennc @BrennanConroy

bjorkstromm commented 8 years ago

Should also functions such as __dnvm_os_runtime_defaults, __dnvm_runtime_bitness_defaults, __dnvm_find_latest and __dnvm_find_package have case insensitive parameters? Now commands like e.g. dnvm install latest -r CoreCLR and dnvm install 1.0.0-beta7 -r CoreCLR will fail.

muratg commented 8 years ago

Yeah, to me it looks like install/upgrade should behave the same way when it comes to -r

cc: @glennc

bjorkstromm commented 8 years ago

I suppose it should be same behavior when it comes to parameters -a|-arch and -OS. At least Windows (ps1) treats all these parameters as case insensitive.

muratg commented 8 years ago

Cool, thanks. Could you send an update? :smile:

bjorkstromm commented 8 years ago

Refactored, and rebased.

Added function __dnvm_to_lower which is called on all occurrences of -r|-runtime, -a|-arch and -OS parameters.

This should now be consistent with Windows.

muratg commented 8 years ago

Awesome, thanks!

muratg commented 8 years ago

@BrennanConroy please review and merge at your convenience. :)

glennc commented 8 years ago

Really happy that we got a PR with a test included :+1:

muratg commented 8 years ago

Agreed with @glennc , thanks @mholo65! @BrennanConroy will merge this in when he's back in the office.