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

populate pshell's env_help with docstring of values #1929

Merged
merged 1 commit into from
Oct 15, 2015

Conversation

jessedhillon
Copy link

This changes pshell to check the docstring of values assigned to env, and if present, use it to as the value for env_help for that value instead of the repr. For example:

Before

Environment:
  app          The WSGI application.
  pp           <function pprint at 0x7f1b37960050>
  registry     Active Pyramid registry.
  request      Active request object.
  root         Root of the default resource tree.
  root_factory Default root factory used to create `root`.
  transction   <module 'transaction' from '/home/jesse/.virtualenvs/boutique/local/lib/python2.7/site-packages/transaction/__init__.pyc'>

After

Environment:
  app          The WSGI application.
  pp           Pretty-print a Python object to a stream [default is sys.stdout].
  registry     Active Pyramid registry.
  request      Active request object.
  root         Root of the default resource tree.
  root_factory Default root factory used to create `root`.
  transaction  Exported transaction functions.  $Id$

SooooOOO nice!

💇 👯 👍

@digitalresistor
Copy link
Member

Please fix coverage in tests, and repush to this PR: https://travis-ci.org/Pylons/pyramid/jobs/84021793

@jessedhillon
Copy link
Author

I'm not familiar with this CI system, can you give any suggestions about what's the issue and how to repro?

@stevepiercy
Copy link
Member

@jessedhillon It's not really the CI, it's the lack of coverage for this feature. To avoid this, always run tests locally before committing. Instructions are in HACKING.txt.

For this specific error, click Details then click on the failing job. Here's the pertinent line.

Note to self, I think I need to finally get around to creating that contributing guidelines file for Pyramid.

@digitalresistor
Copy link
Member

Sure, Travis CI just runs all of our tox environments, you can do so locally too:

  1. Download and install tox (easy_install tox or pip install tox)
  2. At the top-level directory run (to get the coverage report):
    • tox -e py2-cover,py3-cover,coverage

You'll need to make sure that you have both Python 2 (as a binary named python2.7) and Python 3 (as python3.4) installed.

This will run the tests under python 2, collecting information about what lines are covered/not covered, then as python 3 and find out what lines are covered/not covered by tests, and then the last one will merge the two results so that any python 2 or python 3 only code paths don't count against the total lines covered.

@jessedhillon
Copy link
Author

I see what's going on now, thanks. It's complaining that there's no test that touches the case where the object has a falsy docstring. I'll add that test and resubmit. Aside from the test, do you have any comments on the feature itself or the way it's been implemented?

@digitalresistor
Copy link
Member

Nope, looks good to me :-).

@digitalresistor
Copy link
Member

Sorry, I should have been better about including what was wrong, rather than just giving you a link to Travis. That's my bad!

@jessedhillon
Copy link
Author

No worries, I saw the build was failing but didn't really understand the output of the test runner because I was looking for a failing test. I've added a test (did a forced push, didn't want to create a new commit for two lines) that adds None to the env. None.__doc__ is None also, so that hits the falsy case now.

@jessedhillon
Copy link
Author

Pyramid is the only sane framework AFAIAC, so I'm happy to be able to make a contribution and look forward to making more.

@digitalresistor
Copy link
Member

Last but not least to speed up getting this accepted, add a commit that signs CONTRIBUTORS.txt please.

Looks good 👍

@jessedhillon
Copy link
Author

Signed

@digitalresistor
Copy link
Member

@mmerickel This is good to go :-)

@mmerickel
Copy link
Member

Great, I'll merge this when I have a second to backport it to 1.6-branch and update the changelog.

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.

4 participants