Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

If LEIN_OFFLINE env var is set, assume offline mode by default #786

Merged
merged 2 commits into from Sep 18, 2012

Conversation

3 participants
Collaborator

michaelklishin commented Sep 18, 2012

Submitting as a PR for @technomancy to review.

Fixes #678.

Contributor

bruceadams commented on 387e624 Sep 18, 2012

Two comments on this.

The bigger one, that I don't have a solution to, is that you have the environment variable only setting the default. I think it should take precedence; the environment variable should overrule any other settings of "offline?".

The other comment is to allow the environment variable to be set to false. Don't force a user to unset an environment variable to change its meaning. Code something like this:

:offline? (and (not= "false" (System/getenv "LEIN_OFFLINE"))
               (System/getenv "LEIN_OFFLINE"))
Collaborator

michaelklishin replied Sep 18, 2012

Both have crossed my mind.

LEIN_OFFLINE vs :offline? in project.clj

I don't know how often you will find both used at the same time, but I agree that the intent will be to make LEIN_OFFLINE override the setting. Note that LEIN_OFFLINE right now has only one meaning: "we are offline". It is very uncommon to usee :offline? to be set to false in project.clj and expect a user to override it.

LEIN_OFFLINE values

I do not agree that parsing values like "false" or 0 is such a good idea: there is no convention about which env variable values are truthy and which are falsey. A non-trivial number of projects follow the "if it is set, it is true" philosophy. LEIN_OFFLINE=false does not make much sense because most projects won't use the offline setting at all.

So I am not convinced on either of the arguments but if others like them, I will update the PR.

Owner

technomancy commented Sep 18, 2012

We don't currently support ENV_VAR=no anywhere else in Leiningen. I can see the value and would take a patch for it, but it would be in a different patch that would add it across the board for consistency.

As for the precedence question, LEIN_OFFLINE should take precedence over the :offline value in the base project.clj, but not necessarily over a value coming from a profile, since profiles are brought in via an explicit with-profiles invocation, which would generally be considered more explicit and thus higher priority than an environment var. Moving it to the merge call in defproject would accomplish this.

thanks!

Owner

technomancy commented Sep 18, 2012

Looks good; thanks!

@technomancy technomancy added a commit that referenced this pull request Sep 18, 2012

@technomancy technomancy Merge pull request #786 from technomancy/global-offline-flag
If LEIN_OFFLINE env var is set, assume offline mode by default
2c2f132

@technomancy technomancy merged commit 2c2f132 into master Sep 18, 2012

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