Feat: Add testing for utils & delete extraneous code; #145
Conversation
|
I am going to make tests echo the output. I don't think it will break anything, and I want to see my new error messages... |
5-pebbles
force-pushed
the
tests
branch
2 times, most recently
from
2b36c3f
to
81051d9
Compare
5-pebbles
changed the title
They aren't used anywhere...
|
Alright, that's all of the utils tested or deleted. |
5-pebbles
changed the title
lua/nordic/groups/init.lua
Outdated
|
|
||
| local M = {} | ||
|
|
||
| function M.get_groups() | ||
| local native = require('nordic.groups.native').get_groups() | ||
| local integrations = require('nordic.groups.integrations').get_groups() | ||
| local groups = merge(native, integrations) | ||
| local groups = merge_inplace(native, integrations) |
There was a problem hiding this comment.
Pretty sure this should not be a merge inplace? We do not want to modify native here.
There was a problem hiding this comment.
It works because get_groups creates a new table each time; however, I agree it's not very clear. merge and merge_inplace do almost the same thing, so it feels weird to have both.
I could revert that commit or I could make it more clear with something like this:
local native = require('nordic.groups.native').get_groups()
local integrations = require('nordic.groups.integrations').get_groups()
local groups = {}
merge_inplace(groups, integrations)
merge_inplace(groups, integrations)Which ever you think is better.
There was a problem hiding this comment.
Merge and merge inplace are different enough that we would want both, it is very common to have both.
So I think we should:
- Keep both.
- If you want to keep using
merge_inplacein this scenario, please change it the way you suggested. Just a bit more clear what is going on.
There was a problem hiding this comment.
done in: 8c8fff4
| end | ||
| end | ||
|
|
||
| function M.assert_eq(left, right, message) |
There was a problem hiding this comment.
Did you get this function from some other module?
There was a problem hiding this comment.
It's inspired by Rust's assert_eq macro, but all the code is new.
There was a problem hiding this comment.
Sweet. Maybe we should rewrite the theme in Rust?...
There was a problem hiding this comment.
I have been tempted at times: https://github.com/KillTheMule/nvim-rs ;)
|
Nice work on this PR, you really went all out :) |
|
Closes #140. |
|
I think it's good to go! |
|
The one issue I still have with It's no longer being used anywhere, so it won't break anything in the meantime. |
I deleted a substantial portion of utils:
rgb_to_hsv,hsv_to_rbg,darken, andlighten.And the utils that did survive now have some tests.