Skip to content

WIP POC RFC: remove config/session as possible parameters for node #6293

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

Conversation

RonnyPfannschmidt
Copy link
Member

this removes the ability to pass session/config to nodes directly, and in turn exposes a few breakages in hierarchy we implicitly allowed (but shouldn't have to begin with)

I'll work out details of deprecation and moving to this later

@bluetech

This comment has been minimized.

@RonnyPfannschmidt RonnyPfannschmidt changed the base branch from features to master February 12, 2020 21:04
@nicoddemus
Copy link
Member

@RonnyPfannschmidt do you plan to pick this up after 5.4, or before?

@RonnyPfannschmidt
Copy link
Member Author

This is 6.1 stuff

@nicoddemus
Copy link
Member

nicoddemus commented Mar 4, 2020

I meant the deprecation 😁

I'll work out details of deprecation

parent: Optional["Node"] = None,
config: Optional[Config] = None,
session: Optional["Session"] = None,
parent: Union["Node", "Session"],
fspath: Optional[py.path.local] = None,
nodeid: Optional[str] = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we still allow to customize the entire node id?

I know there are items which are derived from other items, for example flake8 items which are derived from normal Python items, but those can be customized by passing a different name already.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actualy an oversight, we should also disallow it quickly for from_parent

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my main intend with this pr was the structural details, so nodeid was not instrumental to the data structure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.

So the main intent is to remove, in 6.1, config, session and nodeid from the Node constructor, as I understand they are the cause of structural problems, correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup

@RonnyPfannschmidt
Copy link
Member Author

closing until i pick up again to clean clutter, its linked in the Node cleanup project

@RonnyPfannschmidt RonnyPfannschmidt deleted the node-only-parent branch June 25, 2022 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: discarded
Development

Successfully merging this pull request may close these issues.

3 participants