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

The names for Inflector functions could be better #21

Open
eeue56 opened this issue Jul 20, 2017 · 11 comments
Open

The names for Inflector functions could be better #21

eeue56 opened this issue Jul 20, 2017 · 11 comments

Comments

@eeue56
Copy link

eeue56 commented Jul 20, 2017

  • Inflector means nothing to me as an Elm developer. Or even as a developer on the whole! I would recommend dropping this subtitle entirely. You can mention that these functions are inspired by inflector functions, but please do not use it as a heading.
  • camelize is not a word that I understand in the context of Elm. toCamelCase would be better.
  • classify does not sense in Elm. Is it converting to a CSS class? We don't have classes in Elm. toCapsCase would be better.
  • humanize seems like a vague definition. Does it fit in with the rest of these functions? toHumanReadable would be a better name
@eeue56 eeue56 changed the title The names for Inflector functions is not good The names for Inflector functions could be better Jul 20, 2017
@jaapz
Copy link
Collaborator

jaapz commented Jul 20, 2017

I'm not sure I agree with the notion that these namings are not understandable in the context of elm. We chose these names because these are inspired from their underscore.string counterparts. I agree that using inflector as a header is bad and we should replace that with something else, or drop it entirely.

I would be fine with moving to more "descriptive" names, if we keep aliases around for the current names.

@eeue56
Copy link
Author

eeue56 commented Jul 20, 2017

Well, to start with, these names are not consistent to String.Extra. Take a look at the converstion functions here: http://package.elm-lang.org/packages/elm-community/string-extra/1.4.0/String-Extra#toSentenceCase. These are the names which you would expect in Elm. senterize or something is not. understandable in the context of Elm means when I am looking for a function, then I would expect to find it fit a given pattern. Right now, String.Extra's naming patterns are on-par with the PHP core library. ize as a function name is extremely Ruby, and does not exist as a pattern in Elm.

Please respond to each of these function names and explain the existing names, if you think the existing one is better:

  • camelize should be toCamelCase
  • classify should be toCapsCase
  • underscored should be toSnakeCase
  • dasherize should be toDashCase
  • humanize should be toHumanReadable

Please remember that just because a function name is familiar to you does not mean it is familiar to the rest of the Elm community. We do not use the same naming as Ruby. Keeping these as aliases is not really an option. We do not want multiple names for one function. Just mention this is called undescored in Ruby and underscore in the docs for toSnakeCase, for example.

@eeue56
Copy link
Author

eeue56 commented Jul 20, 2017

From Slack:

As a general recommendation, please do not name things just because another language calls them something. camelize is not a name I would expect to exist in any published Elm package. Inflector means literally nothing to me.

Especially in the case of classify, the name for that function in Elm just does not make sense. If you are unsure on what name to pick, look at several other languages or libraries, and pick the one that fits best with Elm. If none of them fit Elm, call it a different name. It's totally okay to mention the other names for a function in docs. E.g

{-| Takes in a string and converts it to camel case. The first character will become lowercase, while the first letter of each subsequent word will become uppercase. The spaces between each word will be removed. This is known as `camelize` in Ruby.

.... examples
-}
toCamelCase : String -> String

This way people can still find it via ctrl-f but now their code looks like Elm.

@lorenzo
Copy link
Collaborator

lorenzo commented Jul 20, 2017

I like the new names you proposed, would you like to send a pull request with the changes?

@eeue56
Copy link
Author

eeue56 commented Jul 20, 2017

I don't want to dictate the names -- if @jaapz thinks the existing names are worth keeping, then I'd like to hear them out first before doing it. 😄

@jaapz
Copy link
Collaborator

jaapz commented Jul 20, 2017

I'm fine with new names and understand the reasoning behind it, but making breaking changes by changing the names around without leaving aliases for the old names feels backwards to me. From a practical standpoint it seems to me that keeping aliases would be a small thing to tackle but a big win user-friendly-wise. I personally hate it when people in the npm ecosystem decide to change names around, making me do search and replace work that's not very productive.

Though if you and @lorenzo feel it's better to not keep aliases then by all means propose a pull request and I'll happily approve it.

@eeue56
Copy link
Author

eeue56 commented Jul 20, 2017

@jaapz This is less of an issue in Elm, we can do elm-package diff elm-community/string-extra 3.0.0 4.0.0, and it will tell you exactly what changed. If they want to keep using the old names, then they can lock the deps to a particular version. Polluting the namespace only makes the API worse. We also have the Elm compiler to tell us when a name has changed. Making the API messy and fragmented is exactly how PHP ended up with it's infamous mixed cased library. I would not be happy at all to allow Elm packages fall into the same state.

I'll open a PR in a bit

@jaapz
Copy link
Collaborator

jaapz commented Jul 20, 2017

The issue as a developer is that you still need to change calls to those functions, not that people don't know what changed. I don't think keeping some aliases around will make the API "messy and fragmented". Comparing to the PHP ecosystem is a bit weird, as there is no mixed case issue here. If you feel this strongly that this is the right way and you don't think backwards compatibility is more important here, then like I said, go ahead :)

@eeue56
Copy link
Author

eeue56 commented Jul 20, 2017

@jaapz, any code where this is valid is messy:

alwaysTrue someText = 
  let 
    asCamel = toCamelCase someText
    asCamelToo = camelize someText
  in
    asCamel == asCamelToo

This is not good. Now the developers have to know two names for each function. If you have one developer who is familiar with the camelize function and one who is familiar with toCamelCase, then you will end up with both in one code base. Imagine if you had two functions for adding: + and +++ and they both worked in the same way. Just put the alias name in the docs for that function.

If people care enough that they can't bare to rename their functions, they can make an alias at the cost of their own code base. They can make their own code messy -- and not our packages.

We do not need to care about backwards compatibility in these packages. We have enforced semantic versioning for this exact reason. You can group the naming changes with other changes if you want, but this is not npm and it's not Javascript. We won't cause downtime by changing function names. We'll cause compiler errors to catch these changes trivially.

@ianmackenzie
Copy link
Contributor

What about marking the existing functions as deprecated in the docs (for example "DEPRECATED: use toCamelCase instead"), and then actually removing the functions in some future major release? In addition to providing a grace period for people to update smoothly, I think it's probably a good idea to have fewer major releases to avoid the risk of incompatible version ranges in dependencies (the more major releases of string-extra there are, the more likely you could run into a situation where someDepA depends on string-extra 3.0 <= v <= 4.0 and someDepB depends on string-extra 5.0 <= v <= 6.0).

@eeue56
Copy link
Author

eeue56 commented Jul 20, 2017

Yes, that's reasonable.

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

No branches or pull requests

4 participants