Skip to content

doc: more docs for from_parent #6680

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

@RonnyPfannschmidt RonnyPfannschmidt commented Feb 5, 2020

Fixes #6294.

@RonnyPfannschmidt RonnyPfannschmidt changed the base branch from master to features February 5, 2020 23:00
@blueyed

This comment has been minimized.

@blueyed blueyed changed the title [wip] Fix 6294 more docs for fromparent [WIP] doc: more docs for fromparent Feb 7, 2020
@blueyed blueyed added the type: docs documentation improvement, missing or needing clarification label Feb 7, 2020
@nicoddemus

This comment has been minimized.

@blueyed

This comment has been minimized.

@bluetech

This comment has been minimized.

@RonnyPfannschmidt RonnyPfannschmidt changed the base branch from features to master February 12, 2020 21:06
@RonnyPfannschmidt RonnyPfannschmidt force-pushed the fix-6294-more-docs-for-fromparent branch from 19dd17d to c4b6c93 Compare February 18, 2020 12:40
@RonnyPfannschmidt RonnyPfannschmidt force-pushed the fix-6294-more-docs-for-fromparent branch from f7e804c to 31cd934 Compare February 18, 2020 18:18
@RonnyPfannschmidt RonnyPfannschmidt changed the title [WIP] doc: more docs for fromparent doc: more docs for fromparent Feb 18, 2020
@RonnyPfannschmidt RonnyPfannschmidt force-pushed the fix-6294-more-docs-for-fromparent branch from 31cd934 to 9a727aa Compare February 18, 2020 18:25
@RonnyPfannschmidt
Copy link
Member Author

Deferring squashing due to a hardware failure i can't solve this weekend

@blueyed blueyed changed the title doc: more docs for fromparent doc: more docs for from_parent Feb 24, 2020
@blueyed
Copy link
Contributor

blueyed commented Feb 24, 2020

I think this could also mention how plugins can handle this in compat code.
Should they check for a ´from_parent` attribute?
Some example code for that might be helpful.

@RonnyPfannschmidt RonnyPfannschmidt force-pushed the fix-6294-more-docs-for-fromparent branch from 3e516cb to 3637d9e Compare March 1, 2020 19:35
@nicoddemus
Copy link
Member

I think this could also mention how plugins can handle this in compat code.
Should they check for a ´from_parent` attribute?
Some example code for that might be helpful.

Good idea, took a stab at this in d161bed. @RonnyPfannschmidt is this what you have in mind?

def pytest_pycollect_makeitem(collector, name, obj):
if hasattr(MyItem, "from_parent"):
item = MyItem.from_parent(collector, name="foo")
item.obj = 42
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit strange to me... how are users supposed to write their constructors?

class MyItem(Node):

    def __init__(self, parent, name):
        super().__init__(parent=parent, name=name)
        self.obj = None


# constructed like this:
item = MyItem.from_parent(collector, name="foo")
item.obj = 42

Or are they supposed to override from_parent?

class MyItem(Node):

    def __init__(self, parent, name, obj):
        super().__init__(parent=parent, name=name)
        self.obj = obj

    @classmethod
    def from_parent(cls, parent, name, obj):
        return super().from_parent(parent=parent, name=name, obj=obj) 

# constructed like this:
item = MyItem.from_parent(collector, name="foo", obj=42)

?

Copy link
Member Author

Choose a reason for hiding this comment

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

for initially users will have to override both, we will eventually migrate to a model where they only override from_parent

Copy link
Member

@nicoddemus nicoddemus Mar 4, 2020

Choose a reason for hiding this comment

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

for initially users will have to override both

So I assume you mean the latter sample, right?

we will eventually migrate to a model where they only override from_parent

But how is that possible (to only override from_parent)? Users still need to define their attributes somewhere.

How to you see this playing out in the future?

Also why users even need to implement from_parent at all, given that the framework doesn't call that function? Seems weird to have as prerequisite to override __init__ (which is usually OK in frameworks) and then from_parent, which pytest won't call at all: the user is responsible for creating nodes on the impl of the appropriate hooks (pytest_pycollect_makeitem).

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 update the suggestion to go for attr.s directly, and implementing their own from_parent class?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, I realize this won't work with attr.s because of the superclass relationship.

@blueyed blueyed requested a review from nicoddemus March 27, 2020 01:23
@RonnyPfannschmidt RonnyPfannschmidt merged commit 244c8e4 into pytest-dev:master Apr 10, 2020
chrisrink10 added a commit to basilisp-lang/basilisp that referenced this pull request Jun 11, 2020
This commit corrects the equality operators between builtin collection types to respect equality partitions. Equality partitions are segments of different collections within which collections can be considered equal and between which collections cannot be considered equal. The three equality partitions are: sequential types (vectors, seqs), maps, and sets.

Several other small changes were made in support of that goal:

 * `Map` types now properly support the full Python `Mapping` protocol.
    * `Map.__iter__` now acts like a standard Python `Mapping` type, returning an iterable of keys. Formerly, it was an iterable of `MapEntry` types, which was occasionally convenient, but definitely un-idiomatic in Python. This change doesn't really affect Basilisp code though, since Basilisp always calls `.seq()` on maps to iterate over entries.
    * Removed the `.iteritems()`, `.iterkeys()`, and `.itervalues()` methods which simply forwarded to the base `pyrsistent.PMap` implementation.
    * `Mapping.items()`, `.keys()`, and `.values()` are now supported. All cases relying on the former `__iter__` implementation have generally been updated to use `.items()` (though a few cases were updated to use `.seq()`).
 * All builtin `ISeq` types and `IPersistentVector` have been annotated with the marker interface `ISequential` which is attached to all sequential types in that equality partition.
 * `IPersistentVector` types are no longer considered `Mapping` types, though they are still considered `IAssociative`. It never really made sense to consider them `Mapping` types and it was generally inconvenient since it is not useful to consider vectors as maps in most cases.
 * Removed the custom `Vector.rseq()` implementation since it was undoubtedly no more efficient than Python's `reversed` using the Python `Sequence` protocol.

Two small fixes unrelated to the main changes were also included because I was already in the code:
 * `basilisp.core/rest` now never returns `nil`. Instead, it returns the empty seq in any case it would formerly return `nil`.
 * I fixed a [deprecation with the PyTest plugin](pytest-dev/pytest#6680) that emitted Warnings every time any Basilisp tests ran.

References:
 * [Clojure Collection Model](https://insideclojure.org/2016/03/16/collections/)
 * [Sequences](https://insideclojure.org/2015/01/02/sequences/)
@RonnyPfannschmidt RonnyPfannschmidt deleted the fix-6294-more-docs-for-fromparent branch June 6, 2021 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: docs documentation improvement, missing or needing clarification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve deprecation docs for Node.from_parent
4 participants