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

[WIP] Added more informative output - Addresses #42 #47

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Imaclean74
Copy link

@Imaclean74 Imaclean74 commented Jul 29, 2018

This my first attempt and may well err on the side of being too verbose. Appreciate any feedback on how to make it better.

  • Show the args shiv will be using
  • Notify the user which output file was written and if an automatic entry point has been applied
  • color the ouput from pip in blue to distinguish it from whats coming from shiv. Matches Pipenv for user familiarity
  • Color error messages in red

Here's some example output from a very simple shiv command.

Shiv! 🔪
with args :
	output file : 'test.pyz',
	entry point : 'test',
	python : '/Library/Frameworks/Python.framework/Versions/3.7/bin/python',
	compressed : True
	pip args 'arrow'
Pip installing dependencies to /var/folders/x_/h517ndbj2qx9m6ydt_vtsrv00000gn/T/tmphk62vpm_/site-packages...
Collecting arrow
Collecting python-dateutil (from arrow)
  Using cached https://files.pythonhosted.org/packages/cf/f5/af2b09c957ace60dcfac112b669c45c8c97e32f94aa8b56da4c6d1682825/python_dateutil-2.7.3-py2.py3-none-any.whl
Collecting six>=1.5 (from python-dateutil->arrow)
  Using cached https://files.pythonhosted.org/packages/67/4b/141a581104b1f6397bfa78ac9d43d8ad29a7ca43ea90a2d863fe3056e86a/six-1.11.0-py2.py3-none-any.whl
Installing collected packages: six, python-dateutil, arrow
Successfully installed arrow-0.12.1 python-dateutil-2.7.3 six-1.11.0
Injecting bootstrap code
Creating zip archive
Done, wrote output file to 'test.pyz', entry point is 'test'

- show the args shiv will be using
- Notify the user which output file was written and if an automatic entry point has been applied
- color the ouput from pip in blue to distinguish it from whats coming from shiv. Matches Pipenv for familiararity
- Fixes linkedin#42
@coveralls
Copy link

coveralls commented Jul 29, 2018

Pull Request Test Coverage Report for Build 309

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 22 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-4.1%) to 82.946%

Files with Coverage Reduction New Missed Lines %
.tox/py36/lib/python3.6/site-packages/shiv/info.py 1 95.0%
.tox/py36/lib/python3.6/site-packages/shiv/cli.py 21 77.48%
Totals Coverage Status
Change from base Build 306: -4.1%
Covered Lines: 321
Relevant Lines: 387

💛 - Coveralls

- --ignore-missing-imports looks required for deps with no typedefs.
@Imaclean74
Copy link
Author

@sixninetynine - any chance to take a quick look at this ?

@lorencarvalho
Copy link
Contributor

Hey @Imaclean74

Sorry for the delay in getting back to you. To be completely honest I'm not wild about this change, but given what we are talking about here (UX) there is a lot of subjectivity. Here are my main concerns, perhaps we can find a comfortable middle ground:

1.) I'm not fond of the idea of adding 2 additional dependencies for formatting the output. If shiv were a bigger app with lots of output or more features then I'd be less opposed, but given that it's a pretty small utility with very few deps, I'd like to err on the side of being conservative with our dependency footprint whenever possible.
2.) I'm not a fan of the additional output, and perhaps this is just me being biased b/c I know the tools code

I think a good path forward here would be to drop all this output into debug-level log messages and add an additional command line flag --debug that wires in a StreamHandler. That way folks who want or need this level of verbosity can include that flag. Let me know what you think!

@zvezdan
Copy link
Member

zvezdan commented Aug 21, 2018

I'll just add that we understand the desire to have a more colorful or verbose output, but ...
When adding such features, one needs to consider the use-case outside of our own.
The main purpose of shiv is to be used as a part of other build tooling that is itself a part of the continuous integration pipeline. As such, it does not need to produce the verbose output. In fact, other tools reading its output prefer it that way. Also, as @sixninetynine points out, the overall dependency footprint of the whole build ecosystem needs to stay small.

On the other hand, the users who use shiv as just a command-line utility they directly interact with may want to have more output and colorize it perhaps. The usual way to resolve these two conflicting wishes is to use extra in the Python package specification. That way you can ask to install shiv with an extra to colorize it or make it verbose. For example shiv[colorize] or shiv[verbose], but other users would not get this by default. The default would remain simple build pipeline tool with small number of dependencies.

The extras can be added using extras_require feature in the setup() call in setup.py. That sounds sufficiently easy change to make, but it isn't really. The code needs to be written in such a manner to add the new features if the extras are present, but to gracefully skip them if they are not.

@Imaclean74
Copy link
Author

Hi @sixninetynine - thanks for reviewing.

re 1. Yep - I was thinking the same thing - pretty much everything I'm using crayons for can be done with click - it's just a bit more convenient - I can remove. Similarly - a spinner isn't critical - but can be nice for longer running callouts - to pip for example. I'm fine to remove it.

    • How about just --verbose ? Agreed - the additional output won't always be necessary, particularly for those very familiar wth the tool. My view was - after much time spent struggling with getting pex to do what seemed obvious - it would nice if we can avoid that for shiv - ideally by making it "just work" - where I think its already better than pax, and secondarily by making its inner workings discoverable without a user having to read the code.

Lastly - do we want to keep at least the output file name on the final line ?

Base automatically changed from master to main February 11, 2021 06:28
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.

None yet

4 participants