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

Separate pip's UI from the underlying logic #6541

Open
pradyunsg opened this issue May 26, 2019 · 6 comments
Open

Separate pip's UI from the underlying logic #6541

pradyunsg opened this issue May 26, 2019 · 6 comments
Labels
state: needs discussion This needs some more discussion type: refactor Refactoring code

Comments

@pradyunsg
Copy link
Member

What's the problem this feature will solve?
Currently, pip directly uses logging to log the output on the command line. This means that core logic within pip contains messages that show up on the user. This has various effects, such as complication of code to create better error messages, mixing of "UI" and "business logic" etc.

Describe the solution you'd like
Separate the "core" and "ui" of pip, by using a few objects that act as a layer of abstraction between the two.

I wanted to make note of this idea, even though I'm not sure what this would look like yet or how complicated the implementation/transition would be.

I definitely think this has the potential to improve some bits within pip, especially since error message creation / wording can be better separated from the logic that handles things along with making it explicit at the interface of what the error messages should contain.

Alternative Solutions
Maintain status quo, of logging error messages directly and adding a helper function when the error-messaging logic becomes too complicated.

Additional context
https://www.destroyallsoftware.com/talks/boundaries is a good talk, that's sorta related but not a source/inspiration of this idea.

The closest equivalent to this that I can think of, is taking the idea of the deprecated helper and applying it to pip's entire logging stack.

@pradyunsg
Copy link
Member Author

@pypa/pip-committers Thoughts?

@pfmoore
Copy link
Member

pfmoore commented Jun 7, 2019

In principle this seems like a good idea. How easy it will be in practice is the big question, of course :-)

@cjerdonek
Copy link
Member

Personally, I haven't really had any problems with the status quo with respect to log messages. For example, I've been able to test log messages without any problem that occur in functions (e.g. using pytest's caplog fixture).

If there's an idea for separating e.g. log message text from the surrounding "business logic" code, I think it would be helpful to see an example in a real function. If something other than a logging call were to be used, what would the function call look like if the "business logic" function wanted to communicate (log) that a certain event has happened? And what would it look like to test that compared to testing the log message directly?

@pradyunsg
Copy link
Member Author

Just to set the tone here, if we can figure out a clearer idea for ourselves, on what approach to use for logging, that's good enough. I am not married to the idea in OP. Maybe we won't have a blanket answer but it'll be handy to have this discussion none the less.

TBH, I'm not sure what this might look like either. For all I know, I might just be over-engineering here. :)


(fleshing this out with more thoughts)

Right now, we use multiple ways to handle doing the logging-related logic and message formatting:

  • format a message inline, occasionally with conditionals.
  • helper functions in the same file, often at the end at module-level.
  • use Exception objects (str(e) gives formatted message).
  • deprecated takes information from a context, creates a message and handles the logging as well.
  • more?

I think the first two aren't ideal -- they work but occasionally feel more like a workaround than a scalable solution. IOW, I'd like to avoid having helper functions (like create_env_error_message, invalid_config_error_message and deduce_helpful_msg) and constructing error messages inline in functions (like we do for things like the Python version deprecation messages and parse_editable).

I guess OP suggests moving everything to deprecated style messaging, where the call site doesn't concern itself with the exact message but instead only needs to make sure it passes through the information, bundling the logging calls and the message creation in one place.

Formatting messages in Exception sub-classes also provides the same separation. Additionally, it is a two-stage process (initialize-raise vs call) and bubbles up, so there would be cases where that's more useful.

I'm still not sure how that should look though in terms of organization.

If something other than a logging call were to be used, what would the function call look like if the "business logic" function wanted to communicate (log) that a certain event has happened?

I feel the deprecated decorator or ConfigurationFileCouldNotBeLoaded might be good models. I think it should be something simple, likely only containing a single level of conditionals with any additional conditional logic being on the "business" logic side of things. (I wrote both, so yea biases apply. I did look for other examples in pip's codebase but couldn't find any off-handedly.)

Off-Topic: ConfigurationFileCouldNotBeLoaded should probably get split into 3 classes -- a base class and dedicated classes for the two kinds of errors it is used for.

And what would it look like to test that compared to testing the log message directly?

We would still be able to apply our current testing approaches. In addition, we can also test just the log message generation and/or mock out the "UI" bits and check that the correct information is being passed through.

IOW, we'd have more atomic tests for the "UI" side of things and possibly "business" as well; while the end-to-end stays the same.

@RDIL
Copy link
Contributor

RDIL commented Mar 2, 2020

I don't personally like how pip's UI looks, and could redesign it if you want

@pradyunsg
Copy link
Member Author

@RDIL we have a few UX folks working on pip for ~1 year now, conducting user research that we'd be using to better prioritize and design the UI based on the UX research.

None the less, this issue is more about restructuring of the codebase / responsibilities, not redesigning the UI. That would be #4649. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: needs discussion This needs some more discussion type: refactor Refactoring code
Projects
None yet
Development

No branches or pull requests

4 participants