Skip to content
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

allow @ expressions #269

Closed
wants to merge 3 commits into from
Closed

Conversation

rawwerks
Copy link

@rawwerks rawwerks commented Feb 9, 2024

this commit updates transformers.py to allow @ expressions. would close #268.

@icemac
Copy link
Member

icemac commented Feb 12, 2024

Thank you for your pull request.
We are currently asserting that @ is not supported.
Could you come up with a test asserting that the @-operator works?

@rawwerks
Copy link
Author

rawwerks commented Feb 13, 2024

this is a bit above my pay-grade, so please accept my apologies if i am totally misunderstanding how the tests work.

i'm trying to follow the examples of test_functiondef.py...what about something like this?

def test_RestrictingNodeTransformer__visit_MatMult__1():
    """It allows the `@` operator for matrix multiplication."""
    source_code = """
class MatMulSupport:
    def __matmul__(self, other):
        return "MatMul Operation Allowed"

def test_matmul():
    a = MatMulSupport()
    b = MatMulSupport()
    return a @ b

result = test_matmul()
"""
    result = compile_restricted_exec(source_code)
    assert result.errors == (functiondef_err_msg)

(i'm so unsure of myself that i'm hoping to get feedback before updating the PR)

...or maybe there is a simpler way to just make a foo test...

def foo(a, b):
    return a @ b

@rawwerks
Copy link
Author

ok wait, this is probably the better place to do it: 959ac23

(maybe not the right way, but i think it should work)

@icemac
Copy link
Member

icemac commented Feb 14, 2024

I think I understood the idea of the test and I think I can get it working, but first we need some general paperwork:

Thank you for your contribution.

According to the contributing policies of the zopefoundation organization you need to sign a contributor agreement before any non-trivial change can be merged. For details please consult the Contributing guidelines for zopefoundation projects.

@rawwerks
Copy link
Author

i'm reviewing the document.

in the meantime, hopefully we can do this:

this pull request (#269) by Raymond Weitekamp (rawwerks) is marked with CC0 1.0 Universal.

Copy link
Member

@loechel loechel left a comment

Choose a reason for hiding this comment

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

Please fix the linting errors and get the test green.

transformer.py Line 55-64 must be ammended and __matmul__ be added, or the tests won't pass.

I would suggest to change the test to the more common used structure, where the source_code definition is definied before the test-case.

Thanks for the Pull request.

tests/transformer/operators/test_arithmetic_operators.py Outdated Show resolved Hide resolved
tests/transformer/operators/test_arithmetic_operators.py Outdated Show resolved Hide resolved
@icemac
Copy link
Member

icemac commented Feb 19, 2024

@rawwerks Thank you for putting your changes into public domain. I'll come up with a new PR containing your changes.
@loechel Please the the changes in https://github.com/zopefoundation/.github/blob/master/CONTRIBUTING.md regarding CC0 contributions.

@icemac icemac closed this Feb 19, 2024
@zopefoundation zopefoundation locked as resolved and limited conversation to collaborators Feb 19, 2024
@icemac
Copy link
Member

icemac commented Feb 19, 2024

I created #270 as followup PR.

@loechel
Copy link
Member

loechel commented Feb 19, 2024

@icemac how should I please or approve zopefoundation/.github#8 if it is already merged? I am fine with it, and it makes sense for simple contributions, where people don't want to sign the contributor agreement.

@icemac
Copy link
Member

icemac commented Feb 20, 2024

@loechel Sorry, I did not check what I wrote, I meant: "Please see the changes in …" in case you were not aware of them.

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

Successfully merging this pull request may close these issues.

best approach to allow @/matmult?
3 participants