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.

Sign up for GitHub

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

Jump to bottom

Simplify config internals. #774

Merged
merged 1 commit into from
Jun 10, 2022
Merged

Simplify config internals. #774

merged 1 commit into from
Jun 10, 2022

Conversation

Alexhuszagh
Copy link
Contributor

Simplifies the config.rs and cross_toml.rs to have more reusable functions, and implement logic in terms of these functions, so we can add more config options in the future. This also simplifies maintenance, since we can use 1-line logic for most config variables now.

@Alexhuszagh Alexhuszagh added the no changelog A valid PR without changelog (no-changelog) label Jun 9, 2022
@Alexhuszagh Alexhuszagh requested a review from a team as a code owner June 9, 2022 16:14
@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jun 9, 2022 •
edited
Loading

I'm going to be using this as the basis of fixing #773 (branch for a future PR here), and then implementing a new version of #449 (and also like SSH passthrough), since this makes adding new bool config values much easier.

Emilgardis
Emilgardis reviewed Jun 9, 2022
View reviewed changes
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

awesome!

I don't like the look of having long-named Fn constraints, instead I think it's more readable if they are just impl bounds.

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/cross_toml.rs Show resolved Hide resolved
src/cross_toml.rs Outdated Show resolved Hide resolved
@Alexhuszagh
Copy link
Contributor Author

I've changed all the generic Fn constraints to impl Fn, and then also changed the Vec<String> returns to &[String].

@Alexhuszagh Alexhuszagh force-pushed the config_internals branch 2 times, most recently from 7740bac to 9af7611 Compare June 9, 2022 21:55
@Alexhuszagh Alexhuszagh mentioned this pull request Jun 10, 2022
Merged
@Emilgardis Emilgardis mentioned this pull request Jun 10, 2022
Closed
@Alexhuszagh
Simplify config internals.
cb2abc3 
Simplifies the `config.rs` and `cross_toml.rs` to have more reusable
functions, and implement logic in terms of these functions, so we can
add more config options in the future. This also simplifies maintenance,
since we can use 1-line logic for most config variables now.
@otavio otavio requested a review from Emilgardis June 10, 2022 15:34
@otavio otavio added this to the v0.2.2 milestone Jun 10, 2022
@Alexhuszagh
Copy link
Contributor Author

@Emilgardis Any chance I can get a review on this? I'm working on adding build-std support on top of this. Should I merge this in with the patch for #773 into one PR?

Emilgardis
Emilgardis approved these changes Jun 10, 2022
View reviewed changes
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

lgtm! I do feel there's more simplification possible here, but this is a nice step forward.

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 10, 2022

Build succeeded:

@bors bors bot merged commit 302320e into cross-rs:main Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Emilgardis Emilgardis Emilgardis approved these changes

@otavio otavio otavio approved these changes

Assignees
No one assigned
Labels
no changelog A valid PR without changelog (no-changelog)
Projects
None yet
Milestone
v0.2.2
Development

Successfully merging this pull request may close these issues.
3 participants
@Alexhuszagh @otavio @Emilgardis