ConvertCase trait as a wrapper for other convert case functions #27
Conversation
ConvertCase implements convert_case() which accepts customization options to tweak behaviour of other case conversion functions. Currently, it accepts `number_starts_word` option to have word boundaries when characters in a word change from numeric to alphabetic or vice versa. Other case conversion functions are implemented in terms on convert_case() by passing `number_starts_word` as false by default. ref: withoutboats#18 ref: Peternator7/strum#72
|
Hi @jplatte please review this PR. Let us know if anything needs to be fixed/improved. Also, One thing I would like to get some help on is writing tests in convert_case using macros -- I have just started learning Rust -- if you give some pointers I can give it a go. Thank you. |
|
I have added a macro for the tests. Please check if the implementation is okay or it needs to be changed. Thanks. |
|
I must have been affected by a GitHub bug, because I never received a single notification about this. Will review soon! |
There was a problem hiding this comment.
Thanks for the PR, and sorry for the delay in reviewing it!
As-is, this PR makes the code base a bit harder to understand, and unnecessarily so in my opinion. This is because it introduces dependencies from the existing case transformation traits to the convert_case modules, but also vice-versa.
I would much prefer if all of the actual logic is done in convert_case, and then after this PR is merged it should be pretty simple to merge all of the other traits into ConvertCase (although I'm not certain yet whether I should do this with the next release).
I hope my suggestions below are clear; if they are not please let me know.
| /// | ||
| /// |
There was a problem hiding this comment.
This should just be one empty doc comment line.
| /// ```rust | ||
| /// |
There was a problem hiding this comment.
Swap these two lines (there should not be an empty line at the start of the code block).
| /// let sentence = "Aes128"; | ||
| /// assert_eq!(sentence.convert_case(ConvertCaseOpt { case: Case::ShoutyKebab, number_starts_word: true }), | ||
| /// "AES-128"); | ||
| /// |
There was a problem hiding this comment.
Code block should explicitly be terminated:
| /// | |
| /// ``` |
| UpperCamel, | ||
| } | ||
|
|
||
| pub fn convert_case(s: &str, opt: ConvertCaseOpt) -> String { |
There was a problem hiding this comment.
Why is this a separate free function? I think the body of this should just be moved into the trait implementation below.
| } | ||
| } | ||
| impl ConvertCase for str { | ||
| fn convert_case(&self, opt: ConvertCaseOpt) -> Self::Owned { |
There was a problem hiding this comment.
I think this should be named convert_case_with, and convert_case should be
fn convert_case(&self, case: Case) -> Self::Owned {
self.convert_case_with(ConvertCaseOpt { case, number_starts_word: false })
}| } | ||
|
|
||
| /// supported cases | ||
| pub enum Case { |
There was a problem hiding this comment.
Please derive Clone, Copy and Debug.
| } | ||
|
|
||
| pub fn convert_case(s: &str, opt: ConvertCaseOpt) -> String { | ||
| match opt.case { |
There was a problem hiding this comment.
I think a nicer way to express this function would be transform(s, number_starts_word, case.with_word(), case.boundary()) where
impl Case {
pub(crate) fn with_word(self) -> fn(&str, &mut String) { ... }
pub(crate) fn boundary(self) -> fn(&mut String) { ... }
}represent the actual differing logic that was previously in the individual trait implementations.
| @@ -81,6 +83,8 @@ where | |||
| Lowercase, | |||
| /// The previous cased character in the current word is uppercase. | |||
| Uppercase, | |||
| /// The previous cased character in the current word is numeric | |||
| Numeric, | |||
There was a problem hiding this comment.
I would have expected the logic change in transform to look different, but I also haven't thought about it much. Will review this part in more detail later.
| @@ -17,9 +18,19 @@ pub trait ToKebabCase: ToOwned { | |||
| fn to_kebab_case(&self) -> Self::Owned; | |||
| } | |||
|
|
|||
| pub fn to_kebab(s: &str, number_starts_word: bool) -> String { | |||
There was a problem hiding this comment.
With the proposed changes to the convert_case module, this function doesn't need to exist.
There was a problem hiding this comment.
Same for the other modules.
| convert_case( | ||
| &self, | ||
| ConvertCaseOpt { | ||
| case: Case::Kebab, | ||
| number_starts_word: false, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
With the proposed changes to the convert_case module, this implementation simply becomes
| convert_case( | |
| &self, | |
| ConvertCaseOpt { | |
| case: Case::Kebab, | |
| number_starts_word: false, | |
| }, | |
| ) | |
| self.convert_case(Case::Kebab) |
There was a problem hiding this comment.
Same for the other modules.
|
Ping @ssd532, are you still interested in finishing this? |
|
Closing due to inactivity. |
ConvertCaseimplementsconvert_case()which accepts customization options to tweak the behaviour of other case conversion functions. Currently, it acceptsnumber_starts_wordoption to have word boundaries when characters in a word change from numeric to alphabetic or vice versa.Other case conversion functions are implemented in terms on
convert_case()by passingnumber_starts_wordas false by default.ref: #18
ref: Peternator7/strum#72
Closes #18