Style fixes #4
Style fixes #4
Conversation
| callback(String(line) + "\n"); | ||
| else | ||
| callback(imports.byteArray.toString(line) + "\n"); | ||
| if (SHELL_VERSION <= SHELL_3_34) { |
There was a problem hiding this comment.
If you think 33400 isn't obvious enough, use getShellVersion("3.34.0") instead.
There was a problem hiding this comment.
I'm trying to understand your reasoning... You are okay with saving getShellVersion(Config.PACKAGE_VERSION) to a constant, but not okay with saving 33400 or getShellVersion("3.34.0") to a constant?
Edit: For me it's not a matter of whether 33400 is obvious enough or not, I don't think having magic numbers in multiple places in the code is a good pattern.
There was a problem hiding this comment.
I just don't think that SHELL_3_34 is easier to understand than 33400, which is also a constant. Maybe we can agree on this?
const SHELL_3_34 = getShellVersion("3.34.0");
There was a problem hiding this comment.
I just don't think that
SHELL_3_34is easier to understand than33400, which is also a constant.
If you accidentally deleted a character from SHELL_3_34 you would get an error during parsing/compilation. If you accidentally deleted a character from 33400 you would get an error during run-time (if you happen to trigger the right conditions to see the error).
Maybe we can agree on this?
That's fine, I'll make this change.
| } | ||
|
|
||
| function makeSimpleClass(BaseClass, getSuperArgs, initFn, name) { | ||
| if (shellVersion < 33200) { | ||
| if (SHELL_VERSION < SHELL_3_32) { |
Most of these changes were requested in p-e-w#127 (review) Other changes (line wrapping, const naming, additional consts) came from looking at the rest of the code.
jefferyto
force-pushed
the
style-fixes
branch
from
e1ef586
to
ec7a2e2
Compare
Tested on GNOME Shell 3.38.4 (Ubuntu 21.04). |
|
Works on GNOME 41, too. LGTM. |
Most of these changes were requested in p-e-w#127 (review)
Other changes (line wrapping, const naming, additional consts) came from looking at the rest of the code.