fix: support consecutive capital letters #220
Conversation
googlebot
added
the
cla: yes
synthtool/__main__.py
Outdated
| @@ -70,8 +70,8 @@ def main(synthfile: str, metadata: str, extra_args: Sequence[str]): | |||
| if os.path.lexists(synth_file): | |||
| synthtool.log.debug(f"Executing {synth_file}.") | |||
| # https://docs.python.org/3/library/importlib.html#importing-a-source-file-directly | |||
| spec = importlib.util.spec_from_file_location("synth", synth_file) | |||
| synth_module = importlib.util.module_from_spec(spec) | |||
| spec = util.spec_from_file_location("synth", synth_file) | |||
There was a problem hiding this comment.
I was receiving linting errors locally without switching to this approach (upstream regression perhaps?).
There was a problem hiding this comment.
Honestly that should be a linting error. It's not always guaranteed that submodules are actually loaded when you import a top-level module.
That said, I'd prefer that you instead change the above import to import importlib.util instead. util is too common of a name to be meaningful.
synthtool/gcp/common.py
Outdated
| str2 += " " + chr.upper() | ||
| prev = " " | ||
| str2 = "" | ||
| for i in range(0, len(str)): |
There was a problem hiding this comment.
Nit:
for i, chr in enumerate(str):
would be more idiomatic.
There was a problem hiding this comment.
Also, chr is a built-in, so it's best to avoid that as a variable name. Maybe current to correspond to prev?
synthtool/gcp/common.py
Outdated
| if prev == " ": | ||
| str2 += chr.upper() | ||
| elif re.match(r"[A-Z]", chr): | ||
| # handle "JSONFrom" -> "JSON From" case. |
There was a problem hiding this comment.
I'm not finding these comments helpful. Why does getting the next character solve this case?
synthtool/gcp/common.py
Outdated
| if re.match(r"[A-Z]", chr): | ||
| str2 += " " + chr.upper() | ||
| prev = " " | ||
| str2 = "" |
There was a problem hiding this comment.
I see this is existing code, but output or decamelized would be much more readable than str2.
synthtool/gcp/common.py
Outdated
| str2 += chr.upper() | ||
| elif re.match(r"[A-Z]", chr): | ||
| # handle "JSONFrom" -> "JSON From" case. | ||
| next = "" |
There was a problem hiding this comment.
next is also a built-in, so it's best not to overwrite it. Usually we use a trailing underscore to avoid these kinds of collisions. next_.
synthtool/gcp/common.py
Outdated
| @@ -153,10 +153,22 @@ def _load_partials(self, metadata: Dict): | |||
|
|
|||
| def decamelize(str: str): | |||
There was a problem hiding this comment.
str is a built-in, so we probably shouldn't use it as a variable name. How about value or str_?
synthtool/gcp/common.py
Outdated
| else: | ||
| str2 += chr | ||
| prev = str2[len(str2) - 1] |
There was a problem hiding this comment.
I guess this is to handle when prev should be " "? I'd prefer if the above set a temporary variable for the string to append, then this becomes
output += to_append
prev = to_append
synthtool/__main__.py
Outdated
| @@ -70,8 +70,8 @@ def main(synthfile: str, metadata: str, extra_args: Sequence[str]): | |||
| if os.path.lexists(synth_file): | |||
| synthtool.log.debug(f"Executing {synth_file}.") | |||
| # https://docs.python.org/3/library/importlib.html#importing-a-source-file-directly | |||
| spec = importlib.util.spec_from_file_location("synth", synth_file) | |||
| synth_module = importlib.util.module_from_spec(spec) | |||
| spec = util.spec_from_file_location("synth", synth_file) | |||
There was a problem hiding this comment.
Honestly that should be a linting error. It's not always guaranteed that submodules are actually loaded when you import a top-level module.
That said, I'd prefer that you instead change the above import to import importlib.util instead. util is too common of a name to be meaningful.
The simple algorithm I wrote for decamelize did not support acronyms well (the bug is seen here).
This PR extends the method adding this functionality and adds tests for this edge-case; CC: @steffnay
fixes: #219