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

feat(core): expose configs always #5034

Merged
merged 4 commits into from
Aug 23, 2024
Merged

feat(core): expose configs always #5034

merged 4 commits into from
Aug 23, 2024

Conversation

tisonkun
Copy link
Member

@tisonkun tisonkun commented Aug 22, 2024

Resolve #5033

This demonstrates what will happen. @Xuanwo let's see if it's a good idea.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
/// Sets the time to live of the cache.
///
/// Refer to [`mini-moka::sync::CacheBuilder::time_to_live`](https://docs.rs/mini-moka/latest/mini_moka/sync/struct.CacheBuilder.html#method.time_to_live)
pub time_to_live: Option<Duration>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Duration's default serde is weird (things like { "secs": 1, "nanos": 0 }.

It's better to use humantime-serde but that should be a follow up and breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

I'm considering using an int directly instead of exposing Duration. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Somehow reasonable. In milliseconds precison?

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Overall, this idea looks good to me. However, placing it in services/mod.rs might be difficult to maintain.

Do you think it would be better to have s3_config.rs?

@tisonkun
Copy link
Member Author

Overall, this idea looks good to me. However, placing it in services/mod.rs might be difficult to maintain.

Do you think it would be better to have s3_config.rs?

I move them to core/services/config.rs, not mod.rs.

Each struct is tidy and no more extension can be added. So split to many files (core/services/config/s3.rs in this case) seems only extra burden without certain benefits.

Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: Xuanwo <github@xuanwo.io>
@Xuanwo Xuanwo changed the title feat: expose configs always feat(core): expose configs always Aug 23, 2024
@Xuanwo
Copy link
Member

Xuanwo commented Aug 23, 2024

cc @tisonkun, please take a look.

I prefer to have them in separate files for better readability and easier future code generation.

Copy link
Member Author

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Thank you! You take the time and the result is fine :D

@tisonkun
Copy link
Member Author

@Xuanwo let's give an approval and move forward.

@Xuanwo Xuanwo merged commit dd943a2 into main Aug 23, 2024
263 checks passed
@Xuanwo Xuanwo deleted the expose-configs-always branch August 23, 2024 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose config struct always
2 participants