refactor: first refactor of createReadStream#1118
Merged
danieljbruce merged 6 commits intogoogleapis:mainfrom Jul 14, 2022
Merged
refactor: first refactor of createReadStream#1118danieljbruce merged 6 commits intogoogleapis:mainfrom
danieljbruce merged 6 commits intogoogleapis:mainfrom
Conversation
igorbernstein2
requested changes
Jul 11, 2022
Contributor
igorbernstein2
left a comment
There was a problem hiding this comment.
I think this needs a bit more refactoring. Currently this creates a circular dependency between TableUtils and Table, which is a bad code smell
Contributor
Author
This circular dependency is just a result of some cleanup that wasn't done. I have added a commit so that there aren't circular dependencies anymore which turned out to be a fairly simple change. If you want to change the utils class name then we can do that too. |
Contributor
Author
|
More refactoring is done in the next two PRs. |
Contributor
Author
|
If you want to see a larger refactor then take a look at this PR: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is the first of many PRs to refactor createReadStream. We need to reduce the size of this function so that we can work with it and so that adding retry features is easier.
API surface affected:
requestfunction in index.ts:For tests that look at the API surface:
https://github.com/googleapis/nodejs-bigtable/blob/main/test/table.ts#L539
https://github.com/googleapis/nodejs-bigtable/blob/main/system-test/read-rows.ts#L154
In this PR, we pull out code to calculate the
rangesvariable increateReadStreaminto a separate function. The main motivation for this besides reducing thecreateReadStreamfunction size is to take code, which calculatesrangesinitially and move it into a separate function since this initial calculation has nothing to do with what happens in the rest of the function after that. This conceptually tells the code reader that onlyoptionsis needed to calculate ranges initially and that all code used to calculaterangesinitially can be ignored if the reader is only interested in what happens in the rest of the function.So....
https://github.com/googleapis/nodejs-bigtable/pull/1118/files#diff-6400d8602f0855266c743f4fb031e97ac9cf07f9dcddfba03833b6f04d547a5eL750 to https://github.com/googleapis/nodejs-bigtable/pull/1118/files#diff-6400d8602f0855266c743f4fb031e97ac9cf07f9dcddfba03833b6f04d547a5eL780)
should just be a cut/paste of
https://github.com/googleapis/nodejs-bigtable/pull/1118/files#diff-2490acae1e166b5d2477465b3ee761d0857374c409d215d0363d626c665e80d6R20 to
https://github.com/googleapis/nodejs-bigtable/pull/1118/files#diff-2490acae1e166b5d2477465b3ee761d0857374c409d215d0363d626c665e80d6R48
The only other two minor changes are the changes to the test stub and the change to
createPrefixRange. The test stub changes which include all the changes intest/table.tsare necessary as a result of pulling outrangesinto a utility function and the way that sinon works in the tests.table.createPrefixRangedelegates its functionality toTableUtils.createPrefixRangewhich does the exact same thingtable.createPrefixRangedid (we are just cut/pasting the code), but is something we need to do because of sinon stubs. Basically, sinon gets in the way of just being able to pull code out because it mockscreatePrefixRangefromTable, butTableUtilshas a different copy ofTableso while a test for prefixes should work, in this case the test needs to be slightly modified because it also demands that everything happens inTablewhich shouldn't be necessary. But if this is undesirable then we can consider using a private method instead of a utility class.