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

Windows Environment Vars are converted to all upper case #300

Conversation

weedySeaDragon
Copy link
Contributor

addresses #291
WindowsEnvironmentVars < Environment converts all ENV keys to upper case on initialization.

WindowsPlatform now uses WindowsEnvironmentVars as its environment:

    class WindowsPlatform < UnixPlatform
      ...
      def environment_variables
        WindowsEnvironmentVars.new
      end

Rspec tests for WindowsEnvironmentVars included to be sure that accessing the environment variables by the keys will work and that WindowsEnvironmentVars.keys only has upper case chars.

  • I put the class into aruba/lib/platforms but perhaps it should go somewhere else
  • "WindowsEnvironmentVars.keys only has upper case chars." really means that it only checks to be sure that none of the keys match /[a-z]+/ I think that might not catch lower case unicode chars that don't get 'upper-cased' by [String].upcase
  • Is this is worth including a gem that does unicode checking and 'upper-casing' so that we can be sure to check this? (like unicode_utils) Or should we wait until someone has an issue with it and then deal with it?

def initialize
super
# rubocop:disable Style/EachWithObject
@env = ENV.to_hash.dup.inject({}) { |new_env, (k,v)| new_env[k.to_s.upcase] = v; new_env } #compatible with ruby < 1.9
Copy link
Member

Choose a reason for hiding this comment

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

Let's use if RUBY_VERSION < ' 1.9' here. And make it obvious that we need to change this code if we drop support for ruby < 1.9.3. Please no#duphere.inject` creates a new hash.

@maxmeyer
Copy link
Member

maxmeyer commented Aug 9, 2015

Closed in favor of #303

@maxmeyer maxmeyer closed this Aug 9, 2015
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