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 marks to get rid of the current "marks transfer" mechanism #2363

Closed
nicoddemus opened this Issue Apr 12, 2017 · 6 comments

Comments

Projects
2 participants
@nicoddemus
Member

nicoddemus commented Apr 12, 2017

Marks currently are transferred between nested Python objects after collection, by mutating the objects themselves. For example, marks applied to classes are copied over all the test methods of the class, being added as attributes of the methods themselves.

This approach brings with it some surprising problems and is hard to get it right (#199, #842 and probably many others which have been fixed along the years).

@RonnyPfannschmidt has proposed to get rid of the current mark transfer mechanism in favor of a new one where marks are transfered from parent items to children, instead of mutating the Python objects.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Apr 12, 2017

@nicoddemus there is various bugs about this one already, the basic elements of mistake are that markers are transvered in a mutating way over boundaries they should NEVER cross and finally end up as "kewords" on items

to correctly fix that one we need to make parameterization part of the collection tree, disengage marks from keywords and work out new internal types to represent marks, and potentially expose them to users in the old fashion (or just remove their old exposure alltogether

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Apr 12, 2017

@nicoddemus there is about 10 other issues about marks, we may want to consolidate

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Apr 12, 2017

there is about 10 other issues about marks, we may want to consolidate

I agree, we probably want to at least xref them here (please feel free to do so, I just made a quick search for related issues).

@RonnyPfannschmidt RonnyPfannschmidt moved this from todo to triage in resolve the mark fallout Jun 23, 2017

@RonnyPfannschmidt RonnyPfannschmidt moved this from triage to todo in resolve the mark fallout Jun 23, 2017

@RonnyPfannschmidt RonnyPfannschmidt moved this from todo to planning in resolve the mark fallout Aug 25, 2017

@RonnyPfannschmidt RonnyPfannschmidt moved this from planning to in progress in resolve the mark fallout Mar 27, 2018

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Apr 10, 2018

fixed in #3317 - marker transfer is not needed for the "good" way of accessing markers anymore - the bad way can be removed in a major bump later

@RonnyPfannschmidt RonnyPfannschmidt moved this from in progress to done in resolve the mark fallout Apr 10, 2018

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Apr 10, 2018

Should we open a separate issue to remove the old way of transferring markers and close this one?

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Apr 10, 2018

good thinking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment