fix: Reset lens state when calling Reset - #148
Conversation
islamaliev
left a comment
islamaliev
left a comment
There was a problem hiding this comment.
Looks good overall. I left some minor suggestions/requets
| var _ Action = (*Add)(nil) | ||
| var _ Stateful = (*Add)(nil) | ||
|
|
||
| func (a *Transform) Execute() { |
There was a problem hiding this comment.
suggestion: shouldn't this have name t?
There was a problem hiding this comment.
I think they are all a, for action. Makes copy-pasting them a little easier.
There was a problem hiding this comment.
I though the was some kind of convention that we are trying to stick to.
25th frame: switch to this :)
There was a problem hiding this comment.
I do like this/self 🤣
| require.NoError(a.s.T, err) | ||
| require.False(a.s.T, expectedHasNext) | ||
|
|
||
| a.Expected.Reset() |
There was a problem hiding this comment.
todo: I might be missing something here, but the action is call Transform and calls for some reason Reset which I'm sure if test writer or reader would/should necessarily expect.
Please document it somewhere.
There was a problem hiding this comment.
The expected enumerable is reset, not the actual. This allows the expected results to be iterated through multiple times, e.g. for each node.
I'm not sure this needs documentation and that you just misread glancing through - do you still want a code-comment?
There was a problem hiding this comment.
note: I've merged as I have other stuff I want to do in this repo and a clean main is nice to have, but if you want this let me know and I'll include it in the next PR.
There was a problem hiding this comment.
now that I know it, probably no need from my side, but some other dude might wonder. Up to you
There was a problem hiding this comment.
Will leave as-is then, if someone trips up on it you can fire a 'I told you so' my way :)
|
|
||
| package action | ||
|
|
||
| type TransformReset struct { |
There was a problem hiding this comment.
suggestion: documentation would be nice here
| "github.com/sourcenetwork/lens/tests/modules" | ||
| ) | ||
|
|
||
| func TestAddTransform(t *testing.T) { |
There was a problem hiding this comment.
nitpick: from the test name it's not clear if you are testing adding of a transform or you are testing adding transform, i.e. transform that adds something
There was a problem hiding this comment.
Maybe in this context alone, but all the tests follow this pattern, and it matches the file names.
| Config: model.Lens{ | ||
| Lenses: []model.LensModule{ | ||
| { | ||
| Path: modules.WasmPath4, |
There was a problem hiding this comment.
note: (probably out of scope) the name of lenses are obscure. Would nice to have them more descriptive like module.WasmPathRenameField
There was a problem hiding this comment.
Yes, they are horribly obscure :) They have documentation that shows up in intellisense. I'll make a mental note to clean these up when I do a bit of other clean-up.
| }, | ||
| ), | ||
| }, | ||
| &action.TransformReset{ |
There was a problem hiding this comment.
thought: Would be nice to show what happens if reset isn't called.
There was a problem hiding this comment.
As-in, a new test committed to main? I can try and remember to add one in my next PR.
If you want to see the previous behaviour you can see it in the commit history of this PR.
There was a problem hiding this comment.
Yes a new test committed to main. It would be good documentation that shows what could happen if reset is not used.
3 participants
Relevant issue(s)
Resolves #46
Description
Resets lens state when calling Reset to how it was after configuration but before items were passed through it.