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

[MRG+1] Remove dependency on os.environ from default settings #1829

Merged
merged 1 commit into from May 18, 2017

Conversation

@nyov
Copy link
Contributor

@nyov nyov commented Mar 1, 2016

Avoid loading settings from Environment in scrapy core.
(Then it's not a default setting but a user-defined one, by definition.)
Instead it's better to populate them from the starting shell or an embedding script.

Plug: noticed this while in the mindset of scrapy as a library (#1777 (comment)).

editor = os.environ['EDITOR']
except KeyError: pass
else:
settings['EDITOR'] = editor

This comment has been minimized.

@kmike

kmike Mar 1, 2016
Member

won't this override EDITOR value set in project settings?

This comment has been minimized.

@nyov

nyov Mar 1, 2016
Author Contributor

Yes it will. I think... if you have an ENV setting set as a user, that it should always take precedence.
But I see that would change current behavior. Not sure which should be higher valued, project settings or environment (user terminal/shell setting).

@codecov-io
Copy link

@codecov-io codecov-io commented Mar 1, 2016

Current coverage is 83.43% (diff: 55.55%)

Merging #1829 into master will decrease coverage by 0.01%

@@             master      #1829   diff @@
==========================================
  Files           161        161          
  Lines          8779       8779          
  Methods           0          0          
  Messages          0          0          
  Branches       1288       1288          
==========================================
- Hits           7326       7325     -1   
- Misses         1205       1206     +1   
  Partials        248        248          

Powered by Codecov. Last update cc06b6b...2240f00

@kmike
Copy link
Member

@kmike kmike commented Mar 1, 2016

+1 to remove os.environ usage from default_settings

@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 3, 2016

Thinking about the precedence for EDITOR (Setting over ENV or the other way);
I came to the conclusion that the setting is something of a mistake actually.
In short, it seems quite redundant to me, instead we only need to hardcode our fallback in scrapy/commands/edit.py or scrapy/cmdline.py.

Settings are project-bound, but editor is a user setting. I find no reason why anyone would need to store this in a scrapy setting; this is what a user's terminal environment is for and it belongs in ~/.profile. EDITOR can be nothing but an interactive setting.
Any user working with the scrapy project might have their own preference and would then need to override the project's (other user's) setting first.

Having scrapy care about that setting looks like over-engineering. If I need to store the setting without messing with my ENV, I can use a script in ~/.local/bin/scrapy or similar:

#!/bin/sh
export EDITOR="vim"
scrapy "${@}"

Windows users might not like the shell. But they would need to be on one to type scrapy edit spider, so should also have batch-file support to set this there?

@kmike
Copy link
Member

@kmike kmike commented Sep 9, 2016

+1 to removing EDITOR option and hardcoding a fallback to scrapy commands. This is technically backwards incompatible, but it only affects interactive usage, and there is an easy workaround.

Avoid loading settings from environment in scrapy core.
Instead it's better to populate them from the starting
shell or an embedding script.
@nyov nyov force-pushed the nyov:nyov/editor branch from 9471c70 to 2240f00 Dec 26, 2016
@kmike kmike changed the title Remove dependency on os.environ from default settings [MRG+1] Remove dependency on os.environ from default settings May 18, 2017
@redapple redapple merged commit 8aa2e4f into scrapy:master May 18, 2017
1 of 3 checks passed
1 of 3 checks passed
codecov/patch 55.55% of diff hit (target 83.44%)
Details
codecov/project 83.43% (-0.02%) compared to cc06b6b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@redapple redapple added this to the v1.4 milestone May 18, 2017
@nyov nyov deleted the nyov:nyov/editor branch Jul 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.