-
Notifications
You must be signed in to change notification settings - Fork 350
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add add GitHub helpers for findFilesByFilenameAndRef and findFilesByExtensionAndRef #712
Conversation
Codecov Report
@@ Coverage Diff @@
## master #712 +/- ##
==========================================
+ Coverage 86.78% 86.96% +0.18%
==========================================
Files 55 55
Lines 6582 6673 +91
Branches 586 594 +8
==========================================
+ Hits 5712 5803 +91
Misses 869 869
Partials 1 1
Continue to review full report at Codecov.
|
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 comment re: using sinon for mocking doesn't need to be a blocker, but I wonder if it makes the codebase more maintainable long term.
I've found people really wrestle with nock
, and it might be better to only use it for select things, where we want to be darn sure we've exercised code paths that rely on it.
let path = file.path!; | ||
// strip the prefix if provided | ||
if (prefix) { | ||
const pfix = new RegExp(`^${prefix}[/\\\\]`); |
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 is elegant, I like the approach.
extension: string, | ||
ref: string, |
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.
Should ref be optional, based on the description of the method?
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 originally had it optional as an overload of fileFilesByExtension(extension, prefix?, ref?)
, but then opted to make it a separate explicit method. I'll fix the docs to not make it optional.
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.
Done
const graphql = JSON.parse( | ||
readFileSync(resolve(fixturesPath, 'commits-yoshi-java.json'), 'utf8') | ||
); | ||
const req = nock('https://api.github.com') |
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'm wondering if it would be worth moving some of this logic to sinon
, like here I've been finding that it feels a bit more maintainable than enumerating all of the GitHub endpoints we'll be interacting with.
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 like this a lot. Let me do it in a separate, follow-up PR to avoid muddying up this PR.
Fixes #656 - using the git tree API should be instantaneous and not reliant on the search API.
Fixes #710 - provides extra helpers for finding files by filename/extension within a branch.
Refactors findFiles to use the tree API for listing files.