search

cheshirekow / cmake_format

Source code formatter for cmake listfiles.
GNU General Public License v3.0
958 stars 105 forks source link

Fix kwarg/flag passing for non-lowercase fnnames #98

Closed pingerino closed 5 years ago

pingerino commented 5 years ago

Prior to this change, all function names would be converted to lower-case when looking up the cmdspec for that name, resulting in misses if the function name itself was not all lower case.

I haven't spent much time looking into this, but this fixes my problem and passes the test suite. I can't see any other parts of the code relying on the function name being all lower-case - let me know if this is not the right fix.

cheshirekow commented 5 years ago

Hi, thanks for sharing. Lower casing the keys is intentional as command-name "canonicalization" is an explicit goal of cmake-format. The only way I can see this causing a problem for you is if you are using custom command definitions and you are casing those commands with something other than "upper" or "lower" (i.e. camel?). This isn't something that is supported yet but there is a discussion in #96 describing my planned implementation. Once that is implemented I suggest you take advantage of that mechanism to customize capitalization beyond the upper/lower.

pingerino commented 5 years ago

We're using camel case for some custom commands. Currently I've got the settings set to unchanged in order to avoid updating too much code at once (and I'll need to get consensus from our team on that change).

Another option would be using lower() on the function name when it is set as a key to the cmdspec dictionary. Would you accept that as a PR?

cheshirekow commented 5 years ago

Ah, yes, that's a much better solution. It was my intention that the command specifications are all keyed by lowercase strings and I was planning to enforce that by making all of the keys lower(), as you've stated. If that fits your immediate needs I suggest going with that change so you wont be surprised by any behavior changes in the next release.

pingerino commented 5 years ago

OK. Let me know what you think of this update.