Skip to content

Allow specifying a custom delimiter for sequence fields#29

Open
FuegoFro wants to merge 1 commit into
softprops:masterfrom
FuegoFro:delimiter
Open

Allow specifying a custom delimiter for sequence fields#29
FuegoFro wants to merge 1 commit into
softprops:masterfrom
FuegoFro:delimiter

Conversation

@FuegoFro

Copy link
Copy Markdown

What did you implement:

This allows adding a #[recap(delimiter_regex = "...")] attribute to a field with a sequence-type value which is used when splitting the string. Currently the string is always split on ",", but sometimes lists aren't comma joined. This adds the ability to split strings with an arbitrary regex to handle more situations. This was inspired by https://adventofcode.com/2023/day/4, where splitting on spaces is useful. Note that this was also exposed via the functional bring-your-own-captures API in the form of a new recap::from_captures_with_options method.

Note that this also refactors the derive code to share a common RE instance rather than duplicating it in each method that needs access to the regex. This common location is also where the new field options are stored.

How did you verify your change:

This also adds documentation for the added attribute and a doctest to both demonstrate and verify the behavior.

What (if anything) would need to be called out in the CHANGELOG for the next release:

Since this is a new attribute and behavior, it should probably be added to the changelog.

This allows adding a `#[recap(delimiter_regex = "...")]` attribute to a field with a sequence-type value which is used when splitting the string. Currently the string is always split on `","`, but sometimes lists aren't comma joined. This adds the ability to split strings with an arbitrary regex to handle more situations. This was inspired by https://adventofcode.com/2023/day/4, where splitting on spaces is useful. Note that this was also exposed via the functional bring-your-own-captures API in the form of a new `recap::from_captures_with_options` method.

Note that this also refactors the derive code to share a common `RE` instance rather than duplicating it in each method that needs access to the regex. This common location is also where the new field options are stored.

This also adds documentation for the added attribute and a doctest to both demonstrate and verify the behavior.

Since this is a new attribute and behavior, it should probably be added to the changelog.
FuegoFro added a commit to FuegoFro/recap that referenced this pull request Dec 28, 2023
This is a quick followup to softprops#29 to reduce the duplication of the tuple type `(&'a str, &'a str, Option<&'a FieldOptions>)` by just using `Val<'a>`, which is a struct with the same contents anyway.

Verified by checking that the test still pass.

This doesn't change the public API so no need to update the changelog.
Comment thread recap-derive/src/lib.rs
&format!("RECAP_IMPL_FOR_{}", item.ident.to_string()),
Span::call_site(),
);
let injector = Ident::new(&format!("RECAP_IMPL_FOR_{}", item.ident), Span::call_site());

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is just removing the .to_string() since clippy called it out as unnecessary.

Comment thread recap/src/lib.rs
struct Vars<'a, Iter>(Iter)
where
Iter: IntoIterator<Item = (&'a str, &'a str)>;
Iter: IntoIterator<Item = (&'a str, &'a str, Option<&'a FieldOptions>)>;

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I have a follow-up PR that changes all these duplicated (&'a str, &'a str, Option<&'a FieldOptions>) into Val<'a> but figured that might be better as a separate change. Happy to include it here if you'd rather though!

Comment thread recap/src/lib.rs
Comment on lines +429 to +437
from_iter(re.capture_names().flatten().filter_map(|name| {
caps.name(name).map(|val| {
(
name,
val.as_str(),
field_options.and_then(|fo| fo.get(name)),
)
})
}))

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Some of the reworking of chained methods here also came from clippy's suggestions.

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.

1 participant