dhcp icon indicating copy to clipboard operation
dhcp copied to clipboard

DHCPv6: Other ALL_CAPS constants to CamelCase

Open get9 opened this issue 6 years ago • 5 comments

This changes all other ALL_CAPS constants in various other files in dhcpv6 to CamelCase. These are a bit more controversial changes and IMO don't enhance readability (in some cases, it makes it worse)... Feel free to comment whether you agree or disagree with these changes.

get9 avatar Jul 30 '18 01:07 get9

Codecov Report

Merging #107 into master will not change coverage. The diff coverage is 64.28%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #107   +/-   ##
=======================================
  Coverage   61.31%   61.31%           
=======================================
  Files          66       66           
  Lines        3006     3006           
=======================================
  Hits         1843     1843           
  Misses       1058     1058           
  Partials      105      105
Impacted Files Coverage Δ
dhcpv6/option_archtype.go 76.19% <ø> (ø) :arrow_up:
dhcpv6/option_nii.go 0% <ø> (ø) :arrow_up:
dhcpv6/dhcpv6message.go 48.27% <0%> (ø) :arrow_up:
dhcpv6/utils.go 89.28% <100%> (ø) :arrow_up:
dhcpv6/duid.go 67.94% <66.66%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 23c03c8...3693c7d. Read the comment docs.

codecov[bot] avatar Jul 30 '18 01:07 codecov[bot]

I agree with you that this may not improve readability, rather just make the go linter happier. In cases like DUID_LLT -> DuidLLT, it should probably be DUIDLLT because it's all acronyms, but it would be worse than the other two options. I wouldn't like DuidLlt either. However I don't have a strong opinion on this. @pmazzini what's your thought?

insomniacslk avatar Jul 31 '18 16:07 insomniacslk

I don't have a strong opinion about it either.

pmazzini avatar Aug 01 '18 17:08 pmazzini

So what should we do here?

get9 avatar Aug 11 '18 21:08 get9

hey @get9 , sorry for making you wait. Given that this would be a breaking change (and in https://github.com/insomniacslk/dhcp/issues/123 I committed to doing no breaking changes before versioning - or, I'd add now, unless really necessary), and that there is no strong reasoning in either direction, I'd rather not do this change and delay it until later

insomniacslk avatar Aug 12 '18 22:08 insomniacslk