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

Support #[pyfunction] inside #[pymodule] #693

Closed
wants to merge 1 commit into from

Conversation

davidhewitt
Copy link
Member

It struck me as strange that #[pyfn] exists at all when it should be possible to use #[pyfunction] inside pymodule.

This PR adds exactly that. Maybe this is a road to deprecating / removing #[pyfn] so as to make the overall API simpler? (Just one #[pyfunction] attribute instead of two.)

TODO:

  • Update documentation

@programmerjake
Copy link
Contributor

one reason to have separate #[pyfn] and #[pyfunction] attributes is for when you want a function that's not automatically added to a Python module.

@kngwyu
Copy link
Member

kngwyu commented Dec 18, 2019

#[pymodule]
fn pyfunction_module(_py: Python, module: &PyModule) -> PyResult<()> {
    #[pyfunction]
    fn foobar() -> usize {
        101
    }

    Ok(())
}

Hmm... 🤔
It's confusing for me that pyfunction automatically adds itself to module.
I think we need alternative syntax if we want to do this kind of automatic module declaration.
E.g.,

#[pymodule] 
mod pyfunction_module {
    #[pyfunction]
    fn foobar() -> usize {
        101
    }
}

@davidhewitt
Copy link
Member Author

one reason to have separate #[pyfn] and #[pyfunction] attributes is for when you want a function that's not automatically added to a Python module.

Is there any case where you would write a #[pyfunction] inside a #[pymodule] but not want it automatically added to a module?

This PR doesn't change the fact that #[pyfunction]s outside of a #[pymodule] still need to be manually added.

@davidhewitt
Copy link
Member Author

davidhewitt commented Dec 18, 2019

I think we need alternative syntax if we want to do this kind of automatic module declaration.

I can add mod syntax if that's preferable.

However, my original motivation is that we don't seem to need two differently-named attributes #[pyfn] and #[pyfunction].

I wasn't planning on changing anything besides making it possible to use #[pyfunction] in a functionally-equivalent way to #[pyfn], so that we just need one attribute.

@davidhewitt
Copy link
Member Author

davidhewitt commented Dec 18, 2019

@kngwyu I see what you mean by automatic adding being confusing. I realise now this PR is probably a breaking change, as this is already valid code, but does not add foobar to the module:

#[pymodule]
fn pyfunction_module(_py: Python, module: &PyModule) -> PyResult<()> {
    #[pyfunction]
    fn foobar() -> usize {
        101
    }

    Ok(())
}

I think the solution is instead to add an attribute #[add_to_module(module)], so that instead of:

 #[pyfn(module, "foobar")]
 fn foobar() -> usize {
      101
 }

It would be possible to write:

#[pyfunction]
#[add_to_module(module)]
fn foobar() -> usize {
    101
}

Does this seem better? I've also clarified my rationale for doing this at all on #694.

@kngwyu
Copy link
Member

kngwyu commented Dec 18, 2019

I see what you mean by automatic adding being confusing. I realise now this PR is probably a breaking change, as this is already valid code, but does not add foobar to the module:

I mean that few users expect that the generated code uses unrelated local variable module.
Not about the breaking change.

Does this seem better?

Hmm... 🤔
I'm sorry but I feel diffult to understand why it's better.

@davidhewitt
Copy link
Member Author

davidhewitt commented Dec 18, 2019

Ok 👍

Let's pull back on this idea for now. I still think it would be worth having just #[pyfunction] so that users don't have to learn two different attribute syntaxes for almost exactly the same thing. I'll leave that in #694 .

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.

3 participants