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] bpython support (followup on #270) #1100

Closed
wants to merge 1 commit into from

Conversation

@nyov
Copy link
Contributor

@nyov nyov commented Mar 24, 2015

This is a rebase and revamp of #270
I added support for setting the console/interpreter from scrapy.cfg,
hopefully this might make it mergeable now.

Couldn't find a good place to document this in the docs, so I did it in code :P :P :P

@nyov nyov force-pushed the nyov:bpython_support branch from 511cff9 to 5c0867d Mar 24, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Mar 24, 2015
example:

cat <<EOF >> ~/.scrapy.cfg
[settings]
console = python
EOF

(closes scrapy#270, scrapy#1100)
@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Mar 24, 2015

is it possible to add tests? and can we use a environment variable?

@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 24, 2015

Good question.
If anyone has an idea for an env var for this, and how that should be checked from the code, please share.

I'm a bit stupid here. What should a test for this look like?

@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Mar 24, 2015

@nyov we need to be sure it is respecting the default order and if you implement the env var it is actually overriding that. I guess you can use this, run some dummy or version command and validate the output.

@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 24, 2015

Yeah but the question is - what output to validate. No?

The only thing that changes is the interactive shell. Even if we had an interactive shell in the testsuite, how would the output differ if it went through IPython instead of bpython or plain python?
That's what I'm not figuring here.

@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Mar 24, 2015

/cc @kmike do you know how we can achieve this?

@kmike
Copy link
Member

@kmike kmike commented Mar 24, 2015

Regarding tests: one way to test it is to refactor the code. Create functions _embed_ipython_shell, _embed_bpython_shell, _embed_standard_shell and a function get_shell_embed_func which chooses which shell to embed, based on configuration and import errors (it may return one of these start functions). Then test only get_shell_embed_func.

I'm fine with merging this feature without tests though.

To clarify: I haven't checked this PR.

@nyov nyov force-pushed the nyov:bpython_support branch from 5c0867d to b8f5846 Mar 25, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Mar 25, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 25, 2015

This seems sensible, I have tried to make it that way.
I hope it's okay to modify the function signature of start_python_console.
The current noipython argument seems unused by any scrapy code, so I replaced it with a list of shells to try.

@nyov nyov force-pushed the nyov:bpython_support branch from b8f5846 to 27f2dfe Mar 25, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Mar 25, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Mar 25, 2015

It is taking better form. Can we decorate the _embed_* functions instead of using that test thingy 👅

On _embed_standard_shell if readline isn't present we are assuming that rlcompleter is present.

Don't forget to add documentation.

@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 25, 2015

Right, the test thingy is ugly. I also don't know if NotConfigured is a good choice of exception to raise here, a different name such as StopTest might make more sense.
But a decorator could only wrap the function, I don't see how it might inject an early return.
How would the decorator here look like in your mind?

On _embed_standard_shell if readline isn't present we are assuming that rlcompleter is present.

try...else says: "if we don't have an exception on importing readline, go on and import rlcompleter". So we only import rlcompleter if readline support was present in the first place.
...
Ah I get it now. You meant to say I have an indent failure in the code there. Why don't you just say so.

Don't forget to add documentation.

I was trying to!
Actually I probably won't, unless someone provides a rough draft (what to put where in the docs).
While I don't mind writing docs, I'm reluctant to invest that much unpaid time in someone else's feature request, upfront. (Don't particularly care for it myself, just trying to take down some longstanding open PRs.) Thank you for understanding.

@nyov nyov force-pushed the nyov:bpython_support branch from 27f2dfe to fd9ce2c Mar 25, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Mar 25, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@nyov nyov force-pushed the nyov:bpython_support branch from fd9ce2c to 8ae68e1 Mar 25, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Mar 25, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Mar 26, 2015

Doc 4th paragraph I don't think explaining why bpython is important. Just with adding a link, mentioning we detect it and the order this is done; would be enough.

Decorator:

>>> def _embed_regex(regex, text):
...     import re
...     @wraps(_embed_regex)
...     def wrapper(regex=regex, text=text):
...         return re.findall(regex, text)
...     return wrapper
... 
>>> func = _embed_regex(r'a', 'abc')
>>> func.__name__
'_embed_regex'
>>> func()
['a']
>>> func(r'b')
['b']
# ---
>>> def _embed_fail():
...     import nonexistent
...     @wraps(_embed_fail)
...     def wrapper():
...         return
...     return wrapper
... 
>>> func = _embed_fail()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "<console>", line 2, in _embed_fail
ImportError: No module named nonexistent
@nyov nyov force-pushed the nyov:bpython_support branch from 8ae68e1 to 924ad56 Mar 26, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Mar 26, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 26, 2015

Awwright, added some docs!
Though it seems I fail with the decorators, can still not figure out how to apply them in this context. I'm awfully sorry. :(

@nyov nyov force-pushed the nyov:bpython_support branch from 924ad56 to ad4eadc Mar 27, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Mar 27, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Mar 27, 2015

def _embed_ipython_shell(namespace={}, banner='', **kw):
    """Start an IPython Shell"""
    try:
        from IPython.terminal.embed import InteractiveShellEmbed
        from IPython.terminal.ipapp import load_default_config
    except ImportError:
        from IPython.frontend.terminal.embed import InteractiveShellEmbed
        from IPython.frontend.terminal.ipapp import load_default_config

    @wraps(_embed_ipython_shell)
    def wrapper(namespace=namespace, banner='', **kw):
        config = load_default_config()
        shell = InteractiveShellEmbed(banner1=banner, user_ns=namespace, config=config)
        return shell()
    return wrapper

Then get_shell_embed_func will continue if ImportError and should return _f on try: else:

@nyov nyov force-pushed the nyov:bpython_support branch from ad4eadc to 0f3d7ef Mar 27, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Mar 27, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@nyov
Copy link
Contributor Author

@nyov nyov commented Mar 27, 2015

Ah! Thank you so much. In my mind I thought I needed control-flow back after the wrapper, big fail.
I see it now, obviously. So neat.

This one is on me 🍻

Anything else I failed?

@nyov nyov force-pushed the nyov:bpython_support branch from 0f3d7ef to b5a504a Apr 1, 2015
nyov added a commit to nyov/scrapy that referenced this pull request May 15, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@nyov nyov force-pushed the nyov:bpython_support branch from e444e70 to 34c49fa Jul 10, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Jul 10, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@nyov nyov force-pushed the nyov:bpython_support branch from 34c49fa to e973cf2 Jul 11, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Jul 11, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Jul 17, 2015

Naming the kwarg and the constant the same feels so dangerous 👅

Maybe something like this?

KNOWN_SHELLS = {...}

def f(shells, known_shells=None):
    if known_shells is None:
        known_shells = KNOWN_SHELLS.copy()

I don't know about environment variables; thoughts @kmike ?

self.assertEqual(shell, None)

shell = get_shell_embed_func(['invalid','python'])
assert callable(shell)

This comment has been minimized.

@nramirezuy

nramirezuy Jul 17, 2015
Contributor

This should be self.assertTrue(callable(shell))

@nyov
Copy link
Contributor Author

@nyov nyov commented Jul 17, 2015

Oh, this is a mistake from moving known_shells = {} outside get_shell_embed_func.
How about just uppercasing KNOWN_SHELLS = {} and def get_shell_embed_func(shells, known_shells=KNOWN_SHELLS): - why the .copy()?

@nyov nyov force-pushed the nyov:bpython_support branch from e973cf2 to 8cbaa1c Jul 17, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Jul 17, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be used as the interactive python console, if available.
# (default is ipython, fallbacks in the order listed above)
shell = python
EOF

(closes scrapy#270, scrapy#1100)
@barraponto
Copy link
Contributor

@barraponto barraponto commented Jul 20, 2015

@nyov we usually avoid mutable default arguments due to this: http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments

@nyov nyov force-pushed the nyov:bpython_support branch from 8cbaa1c to 877b19c Jul 21, 2015
nyov added a commit to nyov/scrapy that referenced this pull request Jul 21, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be tried as the interactive python console
# (in above order, unless set here).
shell = python
EOF

(closes scrapy#270, scrapy#1100, scrapy#1301)
@nyov
Copy link
Contributor Author

@nyov nyov commented Jul 21, 2015

Okay, OrderedDict seemed simpler than using tuples :) Here's the update,
added two tests, which depend on bpython and IPython though (let me know if these are overkill and to rather not install those in the test env?)

@cyberplant
Copy link
Contributor

@cyberplant cyberplant commented Jul 25, 2015

Looks good, and works good in Mac OS X with python 2.7.6 and pypy 2.6.0.

However.. the screen is being cleared at the start and I'm not being able to see the scrapy banner. Can we prevent that happening?

Tomorrow will be the sprints at EuroPython 2015 and bpython and scrapy will be there, so maybe I can ask them about that 😄

@nyov
Copy link
Contributor Author

@nyov nyov commented Jul 25, 2015

Nice, hope to see some video coverage of the event later.

To be honest, I didn't give much thought about bpython's screen clearing, but it does make things a bit inconvenient. I couldn't find anything from a quick look about how to maybe pull in custom text into bpython's window at start.
I wonder if the shell could be started before loading the crawler and attaching it to Shell() later, but can't test that now.
Maybe it'd be okay to leave that for a different PR, though? This has been hanging around since early 2013 and, well, I'd hope to see it merged some time or having it rejected on a clear note.

Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be tried as the interactive python console
# (in above order, unless set here).
shell = python
EOF

(closes #270, #1100, #1301)
@nyov nyov force-pushed the nyov:bpython_support branch from 877b19c to c4f5cc9 Aug 15, 2015
@codecov-io
Copy link

@codecov-io codecov-io commented Aug 15, 2015

Current coverage is 82.19%

Merging #1100 into master will increase coverage by +0.02% as of d6223c7

@@            master   #1100   diff @@
======================================
  Files          165     165       
  Stmts         8153    8193    +40
  Branches      1134    1140     +6
  Methods          0       0       
======================================
+ Hit           6699    6734    +35
- Partial        263     264     +1
- Missed        1191    1195     +4

Review entire Coverage Diff as of d6223c7

Powered by Codecov. Updated on successful CI builds.

@nramirezuy nramirezuy changed the title bpython support (followup on #270) [MRG +1] bpython support (followup on #270) Aug 20, 2015
@nramirezuy
Copy link
Contributor

@nramirezuy nramirezuy commented Aug 20, 2015

I don't know if you have to add more test; so the bot smiles at you. 👍

@cyberplant
Copy link
Contributor

@cyberplant cyberplant commented Aug 21, 2015

I didn't know the correct way to add a commit to a foreign PR, so maybe I did it wrong, sorry!

I did another PR (#1444) that takes the help that was being manually printed on console and builds a 'banner' for passing it to the different shells. Tested on standard python, ipython and bpython and works as expected.

@nyov
Copy link
Contributor Author

@nyov nyov commented Aug 21, 2015

@nramirezuy , yeah I knew that would come once I saw the bot :(
And another blocker. Wellll, I'll put it on the todo list. Soon(TM)

@cyberplant, thanks that looks nice! Want me to pull the commit in here, or do you want to keep going with the whole thing in your PR?

@cyberplant
Copy link
Contributor

@cyberplant cyberplant commented Aug 21, 2015

As you wish.. for me, it's the same.

@nyov
Copy link
Contributor Author

@nyov nyov commented Aug 21, 2015

If you're willing to see it through, I'll be glad to let you have it. (And you have the shorter wire to the merge decision people, looks like, so that's an advantage ;)

Too bad I can't hand over the PR or point it at your branch, so yours will be the followup on the followup. 🎉

@nyov nyov closed this Aug 21, 2015
@cyberplant
Copy link
Contributor

@cyberplant cyberplant commented Aug 21, 2015

Understood. 👍

So.. good people, come with me, the party continues here 👉 #1444

nyov added a commit to nyov/scrapy that referenced this pull request Aug 21, 2015
Adds support for configuration of shells from scrapy.cfg
and SCRAPY_PYTHON_SHELL.

config snippet:

cat <<EOF >> ~/.scrapy.cfg
[settings]
# shell can be one of ipython, bpython or python;
# to be tried as the interactive python console
# (in above order, unless set here).
shell = python
EOF

(closes scrapy#270, scrapy#1100, scrapy#1301)
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

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