Skip to content

support for working with class #106

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

Merged
merged 4 commits into from
Mar 23, 2025
Merged

Conversation

AdrianCert
Copy link
Contributor

As discussed on #99 this is changes need to cover this class support

@AdrianCert
Copy link
Contributor Author

@hansthen @JulienPalard could you have a look on this one?

@hansthen
Copy link

Thanks for the Pull Request. I will test this soon (probably next week). I am not the owner of this repository, so I cannot speak for @JulienPalard.

@JulienPalard
Copy link
Owner

I think I may have found an issue caused by instance being stored in the Pipe instance:

from pipe import Pipe


class Factor:
    def __init__(self, n):
        self.n = n

    @Pipe
    def mul(self, iterable):
        return (x * self.n for x in iterable)


mul2 = Factor(2).mul  # stores Factor.mul.instance = Factor(2)
mul10 = Factor(10).mul  # overwrites Factor.mul.instance = Factor(10)

assert list([1, 2, 3] | mul2) == [10, 20, 30]

Instead of storing instance in self we could do as methods are doing: returning a new boundmethod, using something like:

class BoundPipe:
    def __init__(self, pipe, instance, owner):
        self.pipe = pipe
        self.instance = instance
        self.owner = owner

    def __ror__(self, other):
        return self.pipe.function.__get__(self.instance, self.owner)(
            other, *self.pipe.args, **self.pipe.kwargs
        )

Also here by using the function __get__ I don't have to care if it's a normal method, staticmethod, classmethod, whatever: it'll handle it, so it drastically simplifies the rest of the code, Pipe now just needs:

    def __get__(self, instance, owner):
        return BoundPipe(self, instance, owner)

It probably miss a repr on my BoundPipe if you go this way, at least.

(Yes, this means I'd drop the magical detection of classmethod using the argument name, but let's keep things simple.)

With this commit I also include usage of the ruff as linter and formatter
@AdrianCert
Copy link
Contributor Author

@JulienPalard the self.pipe.function.__get__(self.instance, self.owner) is the best.
I update according. Also I hope you don't mind I pushed infrastucture change related (like using ruff project)

@JulienPalard
Copy link
Owner

Also I hope you don't mind I pushed infrastucture change related

I'm 90% OK with them:

  • tox in pyproject.toml : I like it!
  • ruff : why not.
  • But for the build backend, please stick to setuptools, I'm strongly against following the hype of build backends. If I had done so in the past, I would have changed the packaging backend to pipenv, then flit, then poetry, then uv, and I'll change again every other years, ...

Don't misread me, having various build backends is a good thing, they explore new ways to do things, it's like exploring the future: this is good. But having every Python projects change their build backend every other years, no.

@AdrianCert
Copy link
Contributor Author

AdrianCert commented Mar 19, 2025

Also I hope you don't mind I pushed infrastucture change related

I'm 90% OK with them:

* tox in pyproject.toml : I like it!

* ruff : why not.

* But for the build backend, please stick to setuptools, I'm strongly against following the hype of build backends. If I had done so in the past, I would have changed the packaging backend to pipenv, then flit, then poetry, then uv, and I'll change again every other years, ...

Don't misread me, having various build backends is a good thing, they explore new ways to do things, it's like exploring the future: this is good. But having every Python projects change their build backend every other years, no.

@JulienPalard, I agree regards to build backend and I changed according.
Could also check #110?

@AdrianCert AdrianCert requested a review from JulienPalard March 19, 2025 17:08
@AdrianCert AdrianCert requested a review from JulienPalard March 22, 2025 18:58
@JulienPalard JulienPalard merged commit e78c9e4 into JulienPalard:main Mar 23, 2025
5 checks passed
@JulienPalard
Copy link
Owner

Thanks for this great PR @AdrianCert !

@srseshan
Copy link

could you publish this version ? i am seeing the old version of this library in pip repository

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants