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

Add option to export model keys #220

Open
Grimeh opened this issue Sep 1, 2024 · 5 comments · May be fixed by #246
Open

Add option to export model keys #220

Grimeh opened this issue Sep 1, 2024 · 5 comments · May be fixed by #246
Assignees
Milestone

Comments

@Grimeh
Copy link

Grimeh commented Sep 1, 2024

I'm restructuring my project and have noticed that model keys are defined with pub(crate) visibility.
I can see the motivation for this choice and generally agree with it. However it would be helpful to have the ability to make them pub for access from sibling crates.

From a quick look (and my limited experience with proc macros) it seems like it wouldn't be too difficult to add a parameter to the native_db macro to enable something like:

#[native_model(...)]
#[native_db(export_keys = true)]
pub struct MyTable { ... }

Then pub(crate) enum #keys_enum_name from native_db.rs:59 could be changed to something like #keys_visibility enum #keys_enum_name.

pub(crate) enum #keys_enum_name {
#(#keys_enum),*
}

I'm going to make a fork to try this out for my own purposes, would you be keen on this change if I opened a PR?

Grimeh added a commit to Grimeh/native_db that referenced this issue Sep 1, 2024
This adds an optional `export_keys` bool param to the `native_db` proc macro that controls the visibility of the model's secondary key enum.

Default value is `false`, which matches the existing behaviour of setting the visibility of the secondary key enum to `pub(crate)`. Using `#[native_db(export_keys = true)]` changes this to `pub`.

See vincent-herlemont#220 for discussion.
Grimeh added a commit to Grimeh/native_db that referenced this issue Sep 1, 2024
This adds an optional `export_keys` bool param to the `native_db` proc macro that controls the visibility of the model's secondary key enum.

Default value is `false`, which matches the existing behaviour of setting the visibility of the secondary key enum to `pub(crate)`. Using `#[native_db(export_keys = true)]` changes this to `pub`.

See vincent-herlemont#220 for discussion.
@Grimeh
Copy link
Author

Grimeh commented Sep 1, 2024

You can see my proposed impl here: main...Grimeh:export_keys

I added a rudimentary "does this compile with export_keys = true" unit test, but it doesn't test the changed visibility. I'm not sure that's possible to do without creating a separate test crate, which seems like overkill for one or two tests.

I'll open a PR if it looks good after some testing.

@vincent-herlemont
Copy link
Owner

Thank you very much, @Grimeh that's a great idea! You can submit your PR.

@vincent-herlemont vincent-herlemont pinned this issue Sep 7, 2024
@vincent-herlemont vincent-herlemont unpinned this issue Sep 7, 2024
@vincent-herlemont vincent-herlemont added the long-term Long term feature label Oct 5, 2024
@vincent-herlemont
Copy link
Owner

I am closing this issue for now because there is no short-term need in the roadmap, but it will be reopened if needed.

Note: However, a PR is always welcome.

@Grimeh
Copy link
Author

Grimeh commented Oct 5, 2024

Sorry for the delay, just wrapped up a contract that took most of my time.
I'll finally make that PR :)

Grimeh added a commit to Grimeh/native_db that referenced this issue Oct 5, 2024
This adds an optional `export_keys` bool param to the `native_db` proc macro that controls the visibility of the model's secondary key enum.

Default value is `false`, which matches the existing behaviour of setting the visibility of the secondary key enum to `pub(crate)`. Using `#[native_db(export_keys = true)]` changes this to `pub`.

See vincent-herlemont#220 for discussion.
Grimeh added a commit to Grimeh/native_db that referenced this issue Oct 5, 2024
This adds an optional `export_keys` bool param to the `native_db` proc macro that controls the visibility of the model's secondary key enum.

Default value is `false`, which matches the existing behaviour of setting the visibility of the secondary key enum to `pub(crate)`. Using `#[native_db(export_keys = true)]` changes this to `pub`.

See vincent-herlemont#220 for discussion.
@Grimeh Grimeh linked a pull request Oct 5, 2024 that will close this issue
@vincent-herlemont vincent-herlemont removed the long-term Long term feature label Oct 6, 2024
@vincent-herlemont
Copy link
Owner

Thank you for your PR and your work @Grimeh. Consequently, I am placing this issue on the roadmap for 0.8.0.

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 a pull request may close this issue.

2 participants