ConvertCase trait as a wrapper for other convert case functions #27

ssd532 · 2021-01-03T06:29:26Z

ConvertCase implements convert_case() which accepts customization options to tweak the 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: #18
ref: Peternator7/strum#72

Closes #18

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

ssd532 · 2021-01-03T06:55:09Z

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.

ssd532 · 2021-01-03T09:22:09Z

I have added a macro for the tests. Please check if the implementation is okay or it needs to be changed. Thanks.

jplatte · 2021-01-16T13:39:27Z

I must have been affected by a GitHub bug, because I never received a single notification about this. Will review soon!

jplatte

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.

jplatte · 2021-01-16T14:03:26Z

src/convert_case.rs

+///
+///


This should just be one empty doc comment line.

jplatte · 2021-01-16T14:03:49Z

src/convert_case.rs

+/// ```rust
+///


Swap these two lines (there should not be an empty line at the start of the code block).

jplatte · 2021-01-16T14:04:10Z

src/convert_case.rs

+/// let sentence = "Aes128";
+/// assert_eq!(sentence.convert_case(ConvertCaseOpt { case: Case::ShoutyKebab, number_starts_word: true }),
+/// "AES-128");
+///


Code block should explicitly be terminated:

Suggested change

///

/// ```

jplatte · 2021-01-16T14:07:24Z

src/convert_case.rs

+    UpperCamel,
+}
+
+pub fn convert_case(s: &str, opt: ConvertCaseOpt) -> String {


Why is this a separate free function? I think the body of this should just be moved into the trait implementation below.

jplatte · 2021-01-16T14:09:47Z

src/convert_case.rs

+    }
+}
+impl ConvertCase for str {
+    fn convert_case(&self, opt: ConvertCaseOpt) -> Self::Owned {


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 }) }

jplatte · 2021-01-16T14:13:00Z

src/convert_case.rs

+}
+
+/// supported cases
+pub enum Case {


Please derive Clone, Copy and Debug.

jplatte · 2021-01-16T14:15:23Z

src/convert_case.rs

+}
+
+pub fn convert_case(s: &str, opt: ConvertCaseOpt) -> String {
+    match opt.case {


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.

jplatte · 2021-01-16T14:16:17Z

src/lib.rs

@@ -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,


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.

jplatte · 2021-01-16T14:18:07Z

src/kebab.rs

@@ -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 {


With the proposed changes to the convert_case module, this function doesn't need to exist.

Same for the other modules.

jplatte · 2021-01-16T14:19:18Z

src/kebab.rs

+        convert_case(
+            &self,
+            ConvertCaseOpt {
+                case: Case::Kebab,
+                number_starts_word: false,
+            },
+        )


With the proposed changes to the convert_case module, this implementation simply becomes

Suggested change

convert_case(

&self,

ConvertCaseOpt {

case: Case::Kebab,

number_starts_word: false,

},

)

self.convert_case(Case::Kebab)

Same for the other modules.

jplatte · 2021-09-11T21:58:51Z

Ping @ssd532, are you still interested in finishing this?

jplatte · 2023-08-15T08:19:53Z

Closing due to inactivity.

jplatte and others added 8 commits December 29, 2020 14:49

Rename all traits from SomeCase to ToSomeCase

6c384d8

Add .editorconfig

9ac1c74

Forbid unsafe code

894eab6

Rename ToCamelCase => ToUpperCamelCase, ToMixedCase => ToLowerCamelCase

523cf82

Add ToPascalCase as an alias to ToUpperCamelCase

2dbb21f

CI: Use MSRV for build job

f48ec3e

run cargo fmt

3051136

Add a macro for testing ConvertCase

b94a2b8

jplatte reviewed Jan 16, 2021

View reviewed changes

jplatte mentioned this pull request Jan 16, 2021

Release 0.3.4 #29

Closed

jplatte changed the base branch from master to v0.3.x September 11, 2021 21:58

jplatte mentioned this pull request Oct 11, 2021

Added AsXxxCase wrappers #33

Merged

jplatte self-assigned this Apr 21, 2022

jplatte closed this Aug 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ConvertCase trait as a wrapper for other convert case functions #27

ConvertCase trait as a wrapper for other convert case functions #27

ssd532 commented Jan 3, 2021

ssd532 commented Jan 3, 2021

ssd532 commented Jan 3, 2021

jplatte commented Jan 16, 2021

jplatte left a comment

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte Jan 16, 2021

jplatte commented Sep 11, 2021

jplatte commented Aug 15, 2023

ConvertCase trait as a wrapper for other convert case functions #27

ConvertCase trait as a wrapper for other convert case functions #27

Conversation

ssd532 commented Jan 3, 2021

ssd532 commented Jan 3, 2021

ssd532 commented Jan 3, 2021

jplatte commented Jan 16, 2021

jplatte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte commented Sep 11, 2021

jplatte commented Aug 15, 2023