[FEATURE] Pull in any Java or JRuby environment variables that are already set #102

Merged
merged 5 commits into from Oct 9, 2012

Projects

None yet

4 participants

@taylor

Aruba kills java and jruby environment variables currently.

I think the environment variables should be set in everyones features/env.rb file rather than in the lib, but this patch makes it so at least they are merged.

I did not try to handle conflicts.

@mattwynne
Cucumber member

I think the environment variables should be set in everyones features/env.rb file rather than in the lib

Are you saying you think the auto-require of 'aruba/jruby' is too helpful? I don't use this code myself, so I'd be happy to change it to whatever you think is best.

@mattwynne
Cucumber member

Ping @myronmarston, you may be interested in this thread.

@taylor

@mattwynne yes, I find it too helpful. I think that should be documented in the wiki as recommended settings. The end-user/developer can then add that to env.rb.

If it is going to be auto-required then it needs this patch or the user loses any environment settings they have.

@benlangfeld

My thoughts on the ideal approach:

1) Document these particular flags as useful, probably in the README for maximum visibility.
2) Allow aruba/jruby to be required in env.rb, but do not auto-require it.
3) Merge this patch so that if aruba/jruby is required, it does not kill other environment vars.
4) Document the fact that aruba/jruby does not handle conflicting flags and these should be set manually if anything more complex is required.

@mattwynne
Cucumber member

That sounds sensible to me @benlangfeld. The only other thing I'd like to see is a spec/aruba/jruby_spec.rb that pins this behaviour down.

Would anyone like to update this pull request with the changes to the README etc as Ben has suggested? I'll happily merge it if you do.

@benlangfeld

@taylor Please go ahead and complete 1,2&4. Please also add test coverage if it's not significant work.

@mattwynne
Cucumber member

What are you, his boss? ;)

@benlangfeld

PM :)

@myronmarston

I'm cool with this approach as well.

taylor added some commits Jan 4, 2012
@taylor taylor [UPDATE] JRuby auto-require removed; Added tips for JRuby
  * Tips for require or manual env.rb setup added to README
  * Updated jruby.rb to only load config for Java
0ebaf3f
@taylor taylor Merge branch 'master' of git://github.com/cucumber/aruba 15174f6
@taylor taylor [UPDATE] added spec for testing jruby configuration
  * Also added some helper methods for setting and re-setting global constants
d4dca41
@taylor

The first helper method is for setting constants like RUBY_PLATFORM and ENV to something useful during a test and then setting them back to their previous values after the block.

The second helper is used to silence warnings about changing the constant. It can be used to silence other warnings if desired though.

@benlangfeld

Thanks Taylor, looks good.

@mattwynne All yours :)

@benlangfeld

Any chance this might be merged?

@benlangfeld

@mattwynne Anything we need to do before this can be merged?

@mattwynne
Cucumber member

Can you move that with_constants method into a module please and either include it from the spec? Adding global methods like this is a bad idea.

@mattwynne
Cucumber member

Sorry I went to sleep on that. Thanks for your contributions guys.

I've added one comment on the specs, otherwise I'd be happy to merge it in.

@benlangfeld

@taylor Can I have write access to your fork, or would you mind making this change?

@taylor
@mattwynne
Cucumber member

Thanks guys, I'd do it myself but I'm craaazy busy.

@benlangfeld
@mattwynne mattwynne merged commit 4b70f78 into cucumber:master Oct 9, 2012
@mattwynne
Cucumber member

Finally!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment