project-machine / disko

Disk Operations API in Go
Apache License 2.0
13 stars 9 forks source link

Serialize LVType to strings rather than integers. #49

Closed smoser closed 4 years ago

smoser commented 4 years ago

Previously, serializing a LVType gave the helpful 0 or 1. This turns those into more useful values THICK, THIN, THINPOOL.

codecov[bot] commented 4 years ago

Codecov Report

Merging #49 into master will increase coverage by 0.36%. The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #49      +/-   ##
==========================================
+ Coverage   57.00%   57.37%   +0.36%     
==========================================
  Files          15       15              
  Lines        1584     1607      +23     
==========================================
+ Hits          903      922      +19     
- Misses        615      618       +3     
- Partials       66       67       +1     
Impacted Files Coverage Δ
lvm.go 77.77% <83.33%> (+27.77%) :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 eedb2d2...68a0524. Read the comment docs.

smoser commented 4 years ago

@rchamarthy , Please review this wrt a few questions:

  1. is it OK that I changed the values for constants THICK, THIN... ? It felt like the "unknown" should be 0, but that would definitely break anything that had old data.

  2. @tych0 asked "why not just use strings" like:

    type LVType string
    const (
       THIN = LVType("THIN")
       THICK = LVType("THICK")
    )
smoser commented 4 years ago

I backed out the change in values, so that THICK is again 0. I opened up https://github.com/anuvu/disko/issues/50 to discuss/get feedback on strings.

Lets move that conversation there, and review this as it is.

tych0 commented 4 years ago

sounds good, lgtm then.