check-commit: allow foo_bar naming in functions
Needs ReviewPublic

Authored by indygreg on Feb 2 2018, 1:14 PM.

Details

Reviewers
pulkit
durin42
Group Reviewers
hg-reviewers
Summary

nameswithallthewordssmashedtogetherarehardtoread.
especiallyifenglishisnotyourprimarylanguage.

Let's align with the rest of the programming universe and
allow_the_use_of_underscores_in_names.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped
indygreg created this revision.Feb 2 2018, 1:14 PM
pulkit accepted this revision.Feb 2 2018, 1:15 PM
pulkit added a subscriber: pulkit.

I am +1 on this.

durin42 accepted this revision as: durin42.Feb 2 2018, 1:17 PM
durin42 added a subscriber: durin42.

I'm also +1 on this.

yuja added a subscriber: yuja.Feb 3 2018, 3:35 AM

I'm kinda -1 on this because of inconsistency with the 10-year codebase.

av6 added a subscriber: av6.EditedFeb 4 2018, 12:46 AM

As opposed to 11 words smashed together, function names in Mercurial usually are 2 or 3 words (if not just one), which is not at all bad. I think breaking consistency with the existing code base would be worse than making people get used to no-spaces style (the one that python's stdlib usually follows, in fact). So, FWIW, -1 from me.

Should we queue this patch or abandon it?

Should we queue this patch or abandon it?

I wanted to have underscores in names but reading @yuja and @av6 comments about inconsistency, I feel maybe it's not a good idea.

Should we queue this patch or abandon it?

I'm for it, even though it leads to inconsistency. However, we may want to discuss ahead of time what our long-term plan for existing symbols is. Do we eventually want to remove that inconsistency? I took a quick look for examples where it seemed obviously not worth it to rename and it was harder to find good examples than I had expected. Perhaps bail_if_changed and extensions.wrap_function are some of the more frequently used. But most very commonly used functions seem to have short names already. So maybe even if we wanted to eventually make it consistent, it won't be as bad as people have feared? I still don't feel very strongly, but I wanted to highlight what it would mean in practice.

If we loosen the naming requirement, I think a good convention would be to have new files use the *modern* convention and for existing code/files to generally stick to the old convention.

That being said, if someone were to introduce a new function into an existing file and wanted to use the modern names, I wouldn't mind.

I would not like to see global, API breaking rewrites for the sake of rewrites. If we wanted to do a global search and replace on variables inside functions, I'd be OK with that (that won't break API compat). But I'm in no rush to do it.

I would also not like to see patches introducing mixed naming conventions within functions. I think we should try to keep things consistent at definitely the function level and possibly the file level.

If we loosen the naming requirement, I think a good convention would be to have new files use the *modern* convention and for existing code/files to generally stick to the old convention.

That being said, if someone were to introduce a new function into an existing file and wanted to use the modern names, I wouldn't mind.

I would not like to see global, API breaking rewrites for the sake of rewrites. If we wanted to do a global search and replace on variables inside functions, I'd be OK with that (that won't break API compat). But I'm in no rush to do it.

I would also not like to see patches introducing mixed naming conventions within functions. I think we should try to keep things consistent at definitely the function level and possibly the file level.

Sounds good to me. As I said before, we don't seem to have that many functions that have many words in their name, so I don't think the inconsistency would be very noticeable anyway.

Should we queue this patch or abandon it?

I'm for it, even though it leads to inconsistency. However, we may want to discuss ahead of time what our long-term plan for existing symbols is. Do we eventually want to remove that inconsistency? I took a quick look for examples where it seemed obviously not worth it to rename and it was harder to find good examples than I had expected. Perhaps bail_if_changed and extensions.wrap_function are some of the more frequently used. But most very commonly used functions seem to have short names already. So maybe even if we wanted to eventually make it consistent, it won't be as bad as people have feared? I still don't feel very strongly, but I wanted to highlight what it would mean in practice.

extensions.wrap_function is one of the most used function by extensions and I will be happy if we don't rename it. Renaming it will break a lot of extensions, let's prevent it.