This repository has been archived by the owner on Feb 1, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add tests for the volume filter (part of #483) #552
Add tests for the volume filter (part of #483) #552
Changes from 14 commits
db21daf
658f921
91ce0bb
2ec938f
d445752
62b2bc8
886b8fa
70c56c3
4069820
8baa8a9
b398317
c183e30
3e2b2f1
f08a998
d3c5e1a
9379bf5
88da78c
9205c80
b552c96
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this negative value sentinel makes the code a lot more complex.
we now need to keep in our minds the idea of an alternative representation (instruction) of how to create a volume filter config.
we also only use it in ~5 places. not sure i understand the benefit of this -- I had thought that we agreed to remove these sentinel values but I may be mistaken
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we decided either way. Definitely agreed that it's not amazing, but we don't have a great alternative yet, and I'm totally open to another way we can vary this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok to use pointers (via pointy) directly instead of the
-1
sentinels. I think this will help readability.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete line 40 (see below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exchange name should be part of the input -- we don't need the
exchangeName
variable -- if we use a common variable for the inputs then we're not really changing the inputs to the functionwant marketID should be part of the input (if we are constructing the wantVolumeFilter in the test function)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test cases should take a
want
variable. the inputs should be structured so the want values are differentThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do this, but it's a bit messy. Within our current test structure, we'd have to define the testCases within the inner loop of our array for this to work.
The
want
variable requires avolumeFilterConfig
. In a prior comment, you suggested changing thewantFilter
method to take config as parameter - that code change has been made. So, our current approach defines the testCases and then loops over configs and modes. This forecloses referencing the config in thetestCases
array - the config doesn't exist at this point - so we cannot construct thewantVolumeFilter
in the tests as of now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's ok to create the test cases inside the for loop in this situation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a
want
value defined here -- where is the expected output for this test?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wantMarketIDs should be
[]string{"marketID1", "marketID2"},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wantMarketIDs should be a non-empty string array here I think (would be the string inlined value in
firstMarketID
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use a string such as "bgeq7c8v" as the marketID. i.e. inline the value contained in
firstMarketID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wantFilter should go on the test cases (in the way described above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the feeling we had discussed this in the last review -- this cannot use the same
marketIDs
andaccountIDs
variable from the input to construct thewant
value.simple example:
input marketIDs = []string{"marketID1", "marketID1"}
wantMarketIDs = []string{"marketID1"}
therefore these variables cannot be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variables aren't the same, because the
firstMarketID
is de-duped and pre-pended tok.marketIDs
before the comparison. We did talk about this before - I changed the test logic to push it intomakeWantVolumeFilter
. I'm also open to directly inlining this though, that makes sense to me and solves this - will make that change.The reason
wantFilter
cannot go on the test cases is here: Add tests for the volume filter (part of #483) #552 (comment)There's no way for us to construct the
config
, which we loop over, if themarketIDs
andaccountIDs
aren't already defined. But those are defined within the test case. And we can't construct thewant
value without theconfig
. Thoughts on how to solve this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's discuss on a call early next week on Monday or Tuesday and merge this PR in a pair programming session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reviewing later once we have the factory method well tested -- I think we should split these up into separate PRs