lapce icon indicating copy to clipboard operation
lapce copied to clipboard

Move from `String` to `VoltID`

Open JustForFun88 opened this issue 3 years ago • 5 comments

Small, preliminary, yet rather cosmetic improvements that improve the readability of the code and the detection of potential errors (#1881).

The main improvement here is that now Volts is identified not just by a String (which is fraught with errors), but by a structure with its own separate fields.

Further, in the next pull requests, I expect to go further and remove unnecessary cloning and string fetching in many places, since it is obvious that VoltID can be directly compared with VoltInfo or VoltMetadata (for example in HashMaps) without an intermediate buffer, etc. (for example, we can refactor here https://github.com/lapce/lapce/blob/master/lapce-ui/src/window.rs#L334).

P.S. The pull request may seem like a lot, but I actually just added a few tests.

JustForFun88 avatar Dec 30 '22 00:12 JustForFun88

I wouldn't consider calling it VoltIdText an improvement

panekj avatar Dec 30 '22 07:12 panekj

I wouldn't consider calling it VoltIdText an improvement

Yes, you are right, at the moment this is more of a cosmetic improvement, although it improves both the readability of the code and the detection of potential errors (#1881). The main improvement here is that it's not just a string, but a structure with its own separate fields. Further, in the next pull requests, I expect to go further and remove unnecessary cloning and string allocations in many places, since it is obvious that VoltIdText can be directly compared to VoltInfo or VoltMetadata (as example in HashMaps) without an intermediate buffer.

Though I can not absolutely correctly understood you and you mean the name of structure?

JustForFun88 avatar Dec 30 '22 09:12 JustForFun88

I meant going from VoltID to VoltIdText

panekj avatar Dec 30 '22 09:12 panekj

I meant going from VoltID to VoltIdText

Went back to using VoltID

JustForFun88 avatar Jan 03 '23 04:01 JustForFun88

Codecov Report

Merging #1879 (4af9761) into master (97d626d) will increase coverage by 0.57%. The diff coverage is 79.94%.

@@            Coverage Diff            @@
##           master   #1879      +/-   ##
=========================================
+ Coverage    8.49%   9.06%   +0.57%     
=========================================
  Files         130     131       +1     
  Lines       56586   56907     +321     
=========================================
+ Hits         4805    5160     +355     
+ Misses      51781   51747      -34     
Impacted Files Coverage Δ
lapce-data/src/command.rs 0.00% <ø> (ø)
lapce-data/src/config.rs 0.00% <0.00%> (ø)
lapce-data/src/data.rs 0.00% <0.00%> (ø)
lapce-data/src/db.rs 0.00% <0.00%> (ø)
lapce-data/src/plugin.rs 0.00% <0.00%> (ø)
lapce-data/src/proxy.rs 0.00% <0.00%> (ø)
lapce-data/src/settings.rs 0.00% <ø> (ø)
lapce-proxy/src/plugin/catalog.rs 0.00% <0.00%> (ø)
lapce-proxy/src/plugin/lsp.rs 0.00% <0.00%> (ø)
lapce-proxy/src/plugin/mod.rs 0.00% <0.00%> (ø)
... and 12 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov-commenter avatar Jan 03 '23 05:01 codecov-commenter