refactor: enable linter rule QF1004 for 'Use strings.ReplaceAll when possible' #99
Conversation
mwbrooks
added
code health
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #99 +/- ##
==========================================
- Coverage 63.31% 63.28% -0.03%
==========================================
Files 211 211
Lines 22282 22282
==========================================
- Hits 14108 14102 -6
- Misses 7087 7089 +2
- Partials 1087 1091 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@mwbrooks Thanks so much for finding the time to continue these cleanups 💌
I'm approving these changes now but noticed a possible bug that these changes uncovered that might be nice to address in this PR?
Having a number of static checks that can now be counted is exciting for me 🤖
| // TODO, @cchensh, we should get prepared for other non-Git hosts and refactor the create pkg | ||
| func generateGitZipFileURL(templateURL string, gitBranch string) string { | ||
| zipURL := strings.Replace(templateURL, ".git", "", -1) + "/archive/refs/heads/" | ||
| zipURL := strings.ReplaceAll(templateURL, ".git", "") + "/archive/refs/heads/" |
There was a problem hiding this comment.
| zipURL := strings.ReplaceAll(templateURL, ".git", "") + "/archive/refs/heads/" | |
| zipURL := strings.TrimSuffix(templateURL, ".git") + "/archive/refs/heads/" |
🤔 This might've been an existing bug, but we might want to just trim the suffix here?
In this example the template URL for the project is changed in an unexpected fashion:
- https://github.com/slack-samples/example.git-project.git
+ https://github.com/slack-samples/example-projectI don't think we have a test that covers these cases, but it might be nice to cover this case with these changes 🙏 ✨
There was a problem hiding this comment.
Wow, this is a great find! I doubt we have a test for this, so I'm leaning toward merging this PR (linter enablement) and follow-up with a new PR that fixes this issue w/ tests (fix: creating projects from a template url that contains .git).
|
Thanks for the quick review @zimeg. Slowly, we're enabling all of the linters 📝 I'm going to merge this PR and follow-up with a 2nd that fixes the issue that you noticed. I'm doing this so that we have CHANGELOG entries for each (enabling a new linter and fixing an issue). Plus, we can take time to ensure that there are proper tests. ✌🏻 |
Summary
This pull request enables the linter staticcheck rule QF1004: 'Use strings.ReplaceAll instead of strings.Replace with n == -1'.
Requirements