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

Make eligible enums convertable to static strs #4915

Merged
merged 3 commits into from
Jul 23, 2024
Merged

Conversation

SleeplessOne1917
Copy link
Member

I figure it's a good thing to avoid heap allocations where possible. Strum makes this easy for enums, allowing us to convert them to static string slices. It will also be helpful for the leptos ui since I'm using types from api common.

@@ -149,8 +149,7 @@ regex = "1.10.4"
once_cell = "1.19.0"
diesel-derive-newtype = "2.1.2"
diesel-derive-enum = { version = "2.1.0", features = ["postgres"] }
strum = "0.26.2"
strum_macros = "0.26.4"
strum = { version = "0.26.3", features = ["derive"] }
Copy link
Member Author

Choose a reason for hiding this comment

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

Stuff from strum_macros can be imported from strum when the "derive" feature is included. This makes the strum_macros import redundant.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Seems fine. I'll test this out to make sure the generated types for lemmy-js-client don't change.

Also need to run

cargo +nightly fmt

@SleeplessOne1917 SleeplessOne1917 enabled auto-merge (squash) July 23, 2024 02:05
@Nutomic
Copy link
Member

Nutomic commented Jul 23, 2024

Can you show an from your leptos-ui code to see how this is helpful?

@SleeplessOne1917
Copy link
Member Author

I was originally thinking it would be useful for inserting values into query params, although it turns out both arguments for that need to be Strings.

I do anticipate I'll need to use ParamsMap::get and ParamsMap::remove at some point in the future though, so it will be helpful there.

At the very least, I think removing the redundant import will be helpful, and adding another derive trait doesn't add much (if any) complexity and will be useful to anyone else who uses lemmy_api_common in my opinion.

I can undo the derives if you feel it necessary, although I at least think we should keep the removal of the redundant dependency.

@Nutomic Nutomic disabled auto-merge July 23, 2024 14:17
@Nutomic
Copy link
Member

Nutomic commented Jul 23, 2024

Yes removing the extra dependencies is definitely good. But I dont like to add extra derives which arent being used. Otherwise there may be dozens or hundreds of different derives we may add, and they have a cost in terms of maintenance and compile time.

@Nutomic Nutomic enabled auto-merge (squash) July 23, 2024 14:34
@Nutomic Nutomic merged commit db390a2 into main Jul 23, 2024
2 checks passed
Nutomic pushed a commit to sunaurus/lemmy that referenced this pull request Sep 20, 2024
* Make eligible enums convertable to static strs

* Run cargo fmt

* Remove unnecessary derives
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