Ptico / flexus

[discontinued] now dry-inflector
MIT License
2 stars 1 forks source link

Support for lower camel case #3

Open pabloh opened 6 years ago

pabloh commented 6 years ago

inflecto lacks support for lower case camel case conversion like ActiveSupport has, this would useful to have when generating strings for JSON attributes names.

Inflecto.lower_camelize("first_name") # => "firstName"
Inflecto.lower_camelize("name") # => "name"

# Or maybe:

Inflecto.camelize("first_name", lower: true) # => "firstName"
Inflecto.camelize("name", lower: true) # => "name"
Ptico commented 6 years ago

AS second option is shorter, but your suggestion is more verbose, I think we can stick with lower: true and make an alias function lower_camelize.

@abinoam btw, I think we need to release the maintenance version 0.1.0 as soon as we'll have the name and working CI and then start with 0.2.0 with new features

pabloh commented 6 years ago

I don't really have a strong feeling either way, regardless of the API, I think this feature is a must-have.

abinoam commented 6 years ago

I think we can keep mimicking ActiveSupport api as far as it's between the best options. Diverging from that when we find better ways.

pabloh commented 6 years ago

@abinoam, I don't agree about copying ActiveSupport just for the sake of it. To me the AS way is a bit cryptic:

Flexus.camelize("name", true) # What is that 2nd argument about?
Flexus.camelize("name", lower: true) # Using lower case

If you don't know AS API beforehand is quite hard to know what's going on. But I guess is also a matter of terseness vs clearness.

abinoam commented 6 years ago

Hi @pabloh, I agree with you. We shouldn't copying AS just for the sake of it. But we shouldn't NOT copying it just for the sake of it also. If AS syntax is between the best options, we should provide it, even if it's just an alias or helper method.

I prefer "clearness". And we will probably target the new ruby versions with keyword argument support, so a keyword method signature wouldn't hurt at all.

But I also prefer the "less parameters is better". I like the lower_camelize without any parameters. It is less ifs and less code complexity.

I saw some debate about this lower and upper thing at wikipedia:

Some systems prefer camelcase with the first letter capitalised, others not.[1][2][3] For clarity, some incorrectly refer to Pascal Case as upper camel case (initial upper case letter) and lower camel case (initial lower case letter). Some people and organizations, notably Microsoft,[2] use the term camel case only for lower camel case. Source: https://en.wikipedia.org/wiki/Camel_case

Perhaps using camelize was a confusing choice for Rails. Perhaps the reasoning is that "inside" Ruby there's no use for lower camelized words (only when interfacing with other languages like in json).

I'm beginning to think if wouldn't it be better to deprecate camelize in favor of the 2 more verbose and explicit upper_camelize and lower_camelize in a next major version?

pabloh commented 6 years ago

Is not my intention to change the API just to go against AS that would be absurd. I was only pointing out that the way AS handles lower camel case is particularly obscure.

I'm open about changing the API, although It would be interesting to have feedback from other gem owners depending on inflecto.

abinoam commented 6 years ago

Is not my intention to change the API just to go against AS that would be absurd. I was only pointing out that the way AS handles lower camel case is particularly obscure.

Your suggestion have sounded very reasonable.

I'm open about changing the API, although It would be interesting to have feedback from other gem owners depending on inflecto.

The ideia is that MOST people that depends on inflecto will not move 1 cm toward migrating to flexus.

The ones interested in being in sync with the most up to date software will probably enjoy (low effort) changes that are for the good. This kind of change is really low effort. A good search and replace at the text editor and everything will be fine with the changed api.

So, let's keep discussing this and gathering feedbacks and the @Ptico opinions.

pabloh commented 6 years ago

Sorry I should've said gem mainteners using Inflecto AND looking forward to an upgrade.

El 2 nov. 2017 7:57 p. m., "Abinoam P. Marques Jr." < notifications@github.com> escribió:

Is not my intention to change the API just to go against AS that would be absurd. I was only pointing out that the way AS handles lower camel case is particularly obscure.

Your suggestion have sounded very reasonable.

I'm open about changing the API, although It would be interesting to have feedback from other gem owners depending on inflecto.

The ideia is that MOST people that depends on inflecto will not move 1 cm toward migrating to flexus.

The ones interested in being in sync with the most up to date software will probably enjoy (low effort) changes that are for the good. This kind of change is really low effort. A good search and replace at the text editor and everything will be fine with the changed api.

So, let's keep discussing this and gathering feedbacks and the @Ptico https://github.com/ptico opinions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Ptico/flexus/issues/3#issuecomment-341582444, or mute the thread https://github.com/notifications/unsubscribe-auth/AAINXyFwWGn-ESraamUWvEXrc-QfJrteks5sykjigaJpZM4QPkbx .

Ptico commented 6 years ago

The AS API is pretty inconsistent and redundant. So I prefer to not stick with it and make more handy tool.

My suggestion is:

Flexus.camelize('Foo', lower: true)
Flexus.lower_camelize('Foo') # <- Just alias
abinoam commented 6 years ago

There's one problem with this approach. Some people says that "camelize" is always lower camelization (see wikipedia link abov). So it should be the default one (for some people). If you provide a "camelize" with a default behaviour and then we change this default behaviour with lower: true we're opinionating the library to one of the options. We're saying that, for us, camelize is upper camelization. And lower camelization is just an option.

But, if we provide only lower_camelize and upper_camelize there's no doubt what these method do. No need to pass any arguments. And, although the method name is longer, the whole call is shorter because the lack of arguments.

So, if we can do better than ActiveSupport, I think we can go this way. Provide lower and upper. And alias camelize to upper which we'll make us back compatible. Optionally we could issue a deprecation warning.

But, as always, I'm really open to discussion.

Ptico commented 6 years ago

UpperCamelCase is a de-facto standard for Ruby, so ruby programmers expects this behaviour. But I like the verbosity of upper_camelize/lower_camelize. I'm not sure about deprecating, but we can just not mention camelize in docs.

pabloh commented 6 years ago

UpperCamelCase is a de-facto standard for Ruby, so ruby programmers expects this behaviour. But I like the verbosity of upper_camelize/lower_camelize. I'm not sure about deprecating, but we can just not mention camelize in docs.

I was going to point this out, despite the bigger discussion outside the ruby community, I cannot think of any gem or ruby app where camelize doesn't imply upper camel case.

abinoam commented 6 years ago

I understand your point. But when we're talking about lower_camelize is exactly to interact with things outside Ruby like JSON. I mean, people reaching to lower_camelize for json is exactly those who would be confused with camelize being just upper_camelize.

But, I don't have any strong opinion on that and really open to follow the flow.

We can have an intermediate solution as pointed by @Ptico

We define upper_camelize and lower_camelize and document them, so people coming with a fresh mind would just stick with those. We alias camelize to upper_camelize so people migrating from inflecto don't be inflected with the pain of having to change that. But, we just don't document it. And see what happens.

What do you think @pabloh and @Ptico ?

mbj commented 6 years ago

OT: For all the various ways to format an identifier I always wanted an intermediary format to "format from".

This allows to reduce a fair amount of duplication in tests and implementation. As you parse to the intermediary and the intermediary allows to represent itself in snake / weird / lower snake etc.

If I where still maintainig this I'd push for a:

module Flexus
  # Identifier to be formatted.
  # Internal state holds the array of identifier parts 
  class Identifier
    include Adamantium, Concord.new(:names)

    def snake_case
      names.join(' ')
    end
    memoize :snake_case

    def camel_case
      names.map(&:capitalize).join
    end
    memoize :capitalize

    # and all the others here
  end
end

All parsers can be specified from "user input" -> Identifier instance, and all formatters as methods on Identifier this way you should be able reduce the churn significantly.

pabloh commented 6 years ago

The solution proposed by @Ptico is OK to me, but deprecating camelize seems a bit too much, Ruby developers are way too used to that terminology. When I opened this PR I was looking for a method to translate JSON keys besides camelize, since it was obvious that that one wasn't gonna do it.

abinoam commented 6 years ago

@pabloh yes. I'm on with your idea. Just don't document without issueing any deprecation warns. For me is fine.

abinoam commented 6 years ago

@mbj I think I got your point. The "internal" state will be the splitted parts that can be combined to generate any of the final states, including the one it is already. Is this? This would probably significantly simplify the code.

mbj commented 6 years ago

@mbj I think I got your point. The "internal" state will be the splitted parts that can be combined to generate any of the final states, including the one it is already. Is this? This would probably significantly simplify the code.

jup, you explained it in your own words.

mbj commented 6 years ago

I only packaged the code when I extracted it from datamapper 1.x extlib. I only did minimal cycles on it to fix some style stuff and obvious semantic problems. But never had the time to do a real pass.

Ptico commented 6 years ago

I though on this while worked on performance improvements, but this mostly works if we will radically change (or introduce additional one) an API

Ptico commented 6 years ago

I mean, when we have something like:

string = Flexus::String.new('DataMapper::Error')
string.lower_camelize
string.upper_camelize

We will save some computing time having only one parsing stage for all operations. However, we can have some performance penalty if only one operation needed (dasherize for example, use only one String#tr operation which is faster than parsing)

pabloh commented 6 years ago

We should take into account that this kind of conversions will easy become hotspots in many applications. Maybe we should have benchmarks on the CI to test for regressions of that sort?.

2017-11-03 14:12 GMT-03:00 Andrii Savchenko notifications@github.com:

I mean, when we have something like:

string = Flexus::String.new('DataMapper::Error') string.lower_camelize string.upper_camelize

We will save some computing time having only one parsing stage for all operations. However, we can have some performance penalty if only one operation needed (dasherize for example, use only one String#tr operation which is faster than parsing)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Ptico/flexus/issues/3#issuecomment-341768556, or mute the thread https://github.com/notifications/unsubscribe-auth/AAINX0rc07qNONqJqNM5yMdRybDJNLrSks5sy0mNgaJpZM4QPkbx .

mbj commented 6 years ago

We should take into account that this kind of conversions will easy become hotspots in many applications. Maybe we should have benchmarks on the CI to test for regressions of that sort?.

I highly recommend to work out the most easy to understand solution first than to optimize it. Also note that inflecto typically us used during initial metaprogramming / DSL expansion, not runtime.

If you fear to create objects, why use an object oriented language at all? And if you care for speed why use ruby.

Make it correct first, observe speed issues fix them.

Not fear speed issues, write hard to understand code and never know whats going on.

Ptico commented 6 years ago

@mbj the fact that ruby is not the fastest language in the world does not mean that we should not care about performance at all. Inflecto is a "core" lib and I want to make it as fast as possible.

However, maintainability and clean code is an important things. I'll make a PoC next week and let's see how it goes.

mbj commented 6 years ago

@mbj the fact that ruby is not the fastest language in the world does not mean that we should not care about performance at all. Inflecto is a "core" lib and I want to make it as fast as possible.

I've done this talk multiple times. For various teams, maybe my track record makes the following weight more:

Go for maintainability / clarity / correctness first. Than think about performance after measuring deficiencies in real projects.

Do not make up benchmarks, do not compare solution A / B. Wait for real observable performance problems before proactively tampering a design.

pabloh commented 6 years ago

@mbj I completely agree with you, you shouldn't work for the VM and just create less instances to reduce GC pressure. In other words, follow good OOP practices as much as possible and only optimize when you find that some code is a hotspot. So, if there are actual applications suffering from, for instance, JSON key serialization being slow we should consider optimizing.

mbj commented 6 years ago

So, if there are actual applications suffering from, for instance, JSON key serialization being slow we should consider optimizing.

For these you typically should build a static map on bootup anyway. And raise in case you experience an unexpected key. You hopefully have a schema, enforce it.