feat: add addFilesByExtension method & allow leading slash in path #694
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #694 +/- ##
==========================================
+ Coverage 85.38% 85.44% +0.06%
==========================================
Files 49 49
Lines 5945 5972 +27
Branches 531 565 +34
==========================================
+ Hits 5076 5103 +27
Misses 868 868
Partials 1 1
Continue to review full report at Codecov.
|
phated
changed the title
There was a problem hiding this comment.
Only real nit, is that we should be consistent about removing the leading \.
I'm fine with not abstracting the removing of a leading slash into a helper yet.
Disclaimer I always give folks using the search based API, GitHub sometimes doesn't immediately index all files. The approach works, but I think can have some hiccups when a new file is initially added (they eventually correct themselves.)
src/release-pr.ts
Outdated
| @@ -329,7 +329,11 @@ export class ReleasePR { | |||
|
|
|||
| static addPathStatic(file: string, path?: string) { | |||
| if (path === undefined) { | |||
| return file; | |||
| if (file.startsWith('/')) { | |||
There was a problem hiding this comment.
Does it make sense to also check for \?
There was a problem hiding this comment.
Sure! I was originally getting these back from the github API, which always returns /, but I could see someone trying to do a path normalization on windows.
src/github.ts
Outdated
| ): Promise<string[]> { | ||
| let q = `extension:${extension}+repo:${this.owner}/${this.repo}`; | ||
| if (prefix) { | ||
| prefix = prefix.replace(/[/\\]$/, ''); |
There was a problem hiding this comment.
IF we find ourselves doing this too many more times, it might be worth pulling into a helper, removeLeadingSlash?
Could just be in the github.js class I suppose.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #692 Fixes #693 馃