Naming conventions with regex #737
Conversation
googlebot
added
the
cla: yes
|
mmh, I have to think about this. I don't like OTOH, we already use it a one place in the The reason why we used For this change, this should not hold us back for now, and we should proceed for now assuming we use |
pawelsag
force-pushed
the
naming_conventions_with_regex
branch
from
1155efc
to
0bdae87
Compare
pawelsag
force-pushed
the
naming_conventions_with_regex
branch
from
0bdae87
to
240d2cf
Compare
pawelsag
changed the title
There was a problem hiding this comment.
Some initial comments; mostly I think we have to make it clear to the user that the regex are in addition to the thing that is already tested.
In the meantime I'll explore using RE2 instead of std::regex as I like to avoid exceptions (long story). But nothing to worry about for this PR, this is fairly straightforward to change.
| static std::string basic_desc = | ||
| absl::StrCat("Checks that ", Codify("enum", description_type), | ||
| " names use lower_snake_case naming convention" | ||
| " and end with '_t' or '_e'. See ", |
There was a problem hiding this comment.
.. and also the additional optional regular expression.
Otherwise the explanation looks a bit confusing to the user: tests for_t and _e ... and then there is a parameter with regular expressions - it is unclear from seeing that if that is replacing the _t, _e rule or augmenting it.
Similar in the other rules you added regexp: we should update the documentation to include the optional regexp capability.
There was a problem hiding this comment.
Ok so I changed the description as suggested, but I change the phrase to: or also the additional optional regular expression., cause we can use either regex or the original rule, but when regex is enabled we do not validate the old rules.
|
Ok so if I understand you correctly when regex is enabled we should validate the regex, if the regex does not match, we should fall back to the already implemented rules, right? |
|
I was mostly thinking: we always check the built-in rule, and then, in addition, also the regexp. IIRC, that is how you implemented it. Mostly it was important to me that we describe what we do in the description. We could of course think of an either/or approach (if regexp defined, use that, otherwise use built-in rule). |
…iance#737) This is not a desirable solution, but it's better than as-is. There's a couple of reasons for this: 1) lspconfig only enables these commands upon successful attachment of servers, which doesn't really fit their use case. 2) We gain direct access to new Lua APIs for defining user commands, allowing us to leverage Lua callbacks and functions instead of intermediary global variables.
Fixes: #310
But first, the #651 need to be merged