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

Backported Case2. Left 5 tests failing. #46

Merged
merged 3 commits into from
Feb 14, 2019
Merged

Backported Case2. Left 5 tests failing. #46

merged 3 commits into from
Feb 14, 2019

Conversation

am-kantox
Copy link
Contributor

I believe failing tests are very opinionated.

  1) test should preserve path chars (Recase.PathCaseTest)
     test/recase_test/path_case_test.exs:21
     Assertion with == failed
     code:  assert convert("/path case") == "/path/case"
     left:  "path/case"
     right: "/path/case"
     stacktrace:
       test/recase_test/path_case_test.exs:22: (test)

  2) test should not modify extra chars (Recase.PathCaseTest)
     test/recase_test/path_case_test.exs:26
     Assertion with == failed
     code:  assert convert("!#$%^&*(){}[]~`'\"") == "!#$%^&*(){}[]~`'\""
     left:  ""
     right: "!#$%^&*(){}[]~`'\""
     stacktrace:
       test/recase_test/path_case_test.exs:27: (test)

  3) test should not modify extra chars (Recase.PascalCaseTest)
     test/recase_test/pascal_case_test.exs:17
     Assertion with == failed
     code:  assert convert("!#$%^&*(){}[]~`'\"") == "!#$%^&*(){}[]~`'\""
     left:  ""
     right: "!#$%^&*(){}[]~`'\""
     stacktrace:
       test/recase_test/pascal_case_test.exs:18: (test)



  4) test should camel case usual text (Recase.CamelCaseTest)
     test/recase_test/camel_case_test.exs:8
     Assertion with == failed
     code:  assert convert("camel-casE") == "camelCase"
     left:  "camelCasE"
     right: "camelCase"
     stacktrace:
       test/recase_test/camel_case_test.exs:16: (test)

...

  5) test should not modify extra chars (Recase.CamelCaseTest)
     test/recase_test/camel_case_test.exs:19
     Assertion with == failed
     code:  assert convert("!#$%^&*(){}[]~`'\"") == "!#$%^&*(){}[]~`'\""
     left:  ""
     right: "!#$%^&*(){}[]~`'\""
     stacktrace:
       test/recase_test/camel_case_test.exs:20: (test)

1: I doubt I follow why "-foo-bar" → "foo/bar" but "/foo-bar" → "/foo/bar". It’s plain inconsistent (easy to fix though.)
2, 3, 5: If we treat # as a delimiter, it should go away here and there. Use config option to specify delimiters explicitly.
4: How last E becomes lowercased? Why?

@coveralls
Copy link

coveralls commented Feb 14, 2019

Coverage Status

Coverage decreased (-4.08%) to 95.918% when pulling fa24209 on am-kantox:feature/full-unicode-support into d28dbc1 on sobolevn:master.

list

:symbol ->
[all, down, up] = Enum.map([32..127, ?a..?z, ?A..?Z], &Enum.to_list/1)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This treats digits as delimiters as well; maybe it should eliminate digits as well as letters from the list of delimiters.

@sobolevn
Copy link
Member

@am-kantox thanks for this PR!

Now, straight to the point:
1: My idea behind "/foo-bar" → "/foo/bar" was that paths start with / frequently. And it should be preserved. That's why it is not in consistency with other checks. Do you agree on this one?

2,3,5: I guess am just mixing slugs and cases here. It is not really a point of this library to remove any special chars from the strings. Except the explicit separators. If you need to strip these chars, use slugs. So, I agree with you on this one. And I will update docs on making slugs.

4: I guess it is a bug, not sure why this is happening.

@am-kantox
Copy link
Contributor Author

I have fixed a path. Also, I made an explicit set of symbols to be treated as delimiters (confirming your testcase.)

@am-kantox
Copy link
Contributor Author

I am unsure what’s wrong with coverage, it points to @doc so I am a bit confused.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Awesome work @am-kantox! And by "awesome" I mean really awesome.

lib/recase/cases/sentence_case.ex Show resolved Hide resolved
lib/recase/cases/snake_case.ex Show resolved Hide resolved

This module should not be used directly.

**NB!** at the moment has no stop words: titleizes everything
Copy link
Member

Choose a reason for hiding this comment

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

What are "stop words" in this context? We need to document that. Or remove if it does not matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh. There is a link to Wiki https://en.wikipedia.org/wiki/Letter_case#Title_case saying “fruits and vegetables” should be titleized as “Fruits and Vegetables.”

We do not support and remaining all lowercase. That said, we’d better off removing both warning and link to Wiki.

Copy link
Member

Choose a reason for hiding this comment

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

Sound fine, let's do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not in this PR I guess.

@sobolevn sobolevn merged commit aea2153 into wemake-services:master Feb 14, 2019
@sobolevn
Copy link
Member

@am-kantox thanks again! I highly appreciate your work.

@am-kantox am-kantox deleted the feature/full-unicode-support branch February 15, 2019 05:46
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.

3 participants