Closed kurahaupo closed 4 months ago
Please merge the latest feature branch into si_capitalisation
This PR is showing too many changes.
Merging #425 (4777100) into feature (0a0bb41) will not change coverage. The diff coverage is
98.91%
.
@VREMSoftwareDevelopment rebase & conflict resolution done
Thank you for your PR.
Is it possible to split this pull request into 3 smaller pull requests (2.4, 5 and 6 GHz)? It is very hard to review so many changes at once.
Please use Kotlin Coding conventions when changing or adding new code.
Thanks again for the PR.
I've tentatively done this, but it actually increases the number of chunks that need to be reviewed.
Are you reviewing each commit separately, or the whole lot in one go? If the latter it's going to be awkward because combined renaming and editing often confuses "diff" into thinking that a bunch of files have been deleted and a bunch of unrelated files have been added.
I'm wondering if it would be easier to review the list of search-and-replace commands instead? That's only about a dozen lines.
To recreate the changes introduced by this PR:
git checkout feature -b temp425
bash patch.bash
git log -n11 --pretty=oneline
Satisfy yourself that the result is the same as this PR; e.g.:
git log -n11 kurahaupo/si_capitalisation --pretty=oneline
git diff @..kurahaupo/si_capitalisation
Clean up:
git branch -D temp425
git checkout feature
Then read the patch file itself to reassure yourself that all the changes are sane.
If you think that the capitalisation of b
in bandNNGHz or bandwidthNNMHz should be different, it is simple to change the patching script; let me know what you'd like.
note: the point of this is that GHz
and MHz
should have exactly those capitalisations unless there are technical (not just stylistic) reasons for them to be otherwise.
(Previous version: ~patch.bash~)
To be honest, I'm not entirely sure about the resource names; I've included them because there appears to be some automation that converts resource foo_bar
into Kotlin identifier fooBar
, but maybe that's just coincidence?
Feel free to omit those commits if I have this wrong.
Are you reviewing each commit separately, or the whole lot in one go?
https://github.com/VREMSoftwareDevelopment/WiFiAnalyzer/pull/425/files
The code changes do not comply to Kotlin coding conventions
Reference documentation:
Is there any point to me continuing with this?
I only started this because I found the mix of "Ghz", "ghz", "GHZ", "Mhz", "mhz", and "MHZ" very distracting and therefore harder to read. None of those is compliant with the Metric Convention.
You haven't given any explanation or examples of how the patch breaches each of the coding conventions, but it seems like your primary objection is that changing the capitalisation of "MHz" and "GHz" is not allowed by the Kotlin coding conventions. If that's not you primary objection then please say so.
Coding conventions are supposed to make code easier to read and easier to manage, but the key is understanding how rigorously they should be applied, especially when they directly conflict with other conventions. (The Metric Convention is used by more than 7 billion people, which to my mind means it takes precedence over lesser conventions.)
Is there any accommodation that would satisfy both of us?
Making code more readable is always a plus. Making code more readable while violating coding standards is a minus.
It is recommended that:
By making a smaller PR, will allow to comment exactly on the changes that violate Coding Standards
You haven't given any explanation or examples of how the patch breaches each of the coding conventions, but it seems like your primary objection is that changing the capitalisation of "MHz" and "GHz" is not allowed by the Kotlin coding conventions. If that's not you primary objection then please say so.
It depends where, some place it is okay to have GHz
, some other once it is not.
Commonly known and easy-to-follow coding conventions are vital for any programming language. Here we provide guidelines on the code style and code organization for projects that use Kotlin.
Two most popular IDEs for Kotlin - IntelliJ IDEA and Android Studio provide powerful support for code styling. You can configure them to automatically format your code in consistence with the given code style.
By making a smaller PR, will allow to comment exactly on the changes that violate Coding Standards
Even replacing one item would make a patch well in excess of 100 lines, so I don't see how a "smaller PR" would help. (I followed the suggestion to subdivide into 2GHz, 5GHz & 6GHz, and I'm pretty sure that's made the situation worse rather than better.)
Rather than review the whole PR, how about just commenting on these:
within | decl | old name | new name | ok / not ok |
---|---|---|---|---|
package | val | availableGHZ6 |
available6GHz |
? |
class | val | channelAvailableGhz6 |
channelAvailable6GHz |
? |
class | fun | channelAvailableGHZ6 |
channelAvailable6GHz |
? |
class | val | channelAvailableTitleGhz6 |
channelAvailableTitle6GHz |
? |
R.id | channel_available_title_ghz_6 |
channel_available_title6_g_hz |
? | |
class | fun | channelsGHZ6 |
channels6GHz |
? |
class | val | expectedGHZ6 |
expected6GHz |
? |
enum class | ? | GHZ6 |
band6GHz |
? |
enum class | val | ghz6 |
is6GHz |
? |
class | val | wiFiChannelGHZ6 |
wiFiChannel6GHz |
? |
package | class | WiFiChannelCountryGHZ6 |
WiFiChannelCountry6GHz |
? |
I can't see how the symbol GHZ6
is declared, so I don't know what its kind is, and therefore I don't know which style rule applies to it. Is it perhaps an enum?
I've written _g_hz
in the resource names because I assume that snake_case resource names map to camelCase names, and that's what I need to match GHz
elsewhere in the code. If that assumption is wrong I'll gladly drop that transformation because it looks even uglier than the original.
I suspect that having val channelAvailableGhz6
and fun channelAvailableGHZ6
is not ideal. Should one of them be renamed to make the val/fun distinction clearer?
Install Kotlin Coding conventions in the Android IDE
Run (Analyze) Android IDE Inspect Code -> Analyzer
Take a look at the Inspection Results Kotlin Section
All the Kotlin Code issues should be available there
What does this implement/fix? Please describe.
Other info
MHz
&GHz
to match that specification.Not fixed
2GHz
should be changed to2400MHz
Where has this been tested?