-
-
Notifications
You must be signed in to change notification settings - Fork 530
refactor session #1145
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
refactor session #1145
Conversation
Wow, that's a big one :) Would you like feedback already about the direction this goes in? I'll have some time tomorrow afternoon to have closer look, if it makes sense already. |
@obestwalter yeah hence opening it 👍 even though not 100% ready yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get a chance to read through it all -- got about halfway through and realized github probably doesn't show moves well so I'll probably need to look using the cli
Overall the parts I looked at seemed fine, moving the reporter to a global seems risky, especially now that we;re doing parallel things
src/tox/_pytestplugin.py
Outdated
|
||
|
||
@pytest.fixture | ||
def newmocksession(mocksession, newconfig): | ||
def newmocksession_(args, source, plugins=()): | ||
mocksession.config = newconfig(args, source, plugins=plugins) | ||
config = newconfig(args, source, plugins=plugins) | ||
mocksession.__init__(config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, I've never seen this before -- I assume it just re-initializes the object? is this well defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just a function call AFAIK 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite get, why this change is necessary at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because we want to reset the report, calculated session venvs when a new config is loaded 👍 can use the _reset path though.
I only had time to skim through, but I like the general direction of it. I'll try to carve out some more time tomorrow to play with it. |
I'd say let's run with this after the comments are addressed and test failures are fixed. Stripping down the several Main thing is we keep backwards compatibility regarding the main objects that people use from other tools / plugins. |
This looks OK for now. I'm not 100% settled on the reporting thing, but we can take it for a test drive. Maybe instead we should just use the logging framework which we kinda mimic. But this can be a subsequent PR to migrate to that. |
It looks like this PR causes pip's CI to fail. Here is the traceback:
Apparently this is because |
Also,
|
While looking into doing #998 I realized the concept of env logs and actions have been heavily overlooked, furthermore the reporting should not be tied to the session (as we should report before the session object is constructed). This PR tries to fix all this and split up the longer and longer getting
session.py
.