new recipe default branch main instead of master #451
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: toolshed/abra#451
Reference in New Issue
Block a user
No description provided.
Delete Branch "defaultBranch"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Just saving my future self the trouble of searching "how to rename git branch" every-time I create a new recipe
tysm for dipping into this, definitely important work!
@ -14,2 +15,4 @@if err != nil {return fmt.Errorf("git init: %s", err)}MasterToMain(repo)Since
Initreturns anerror,MasterToMainshould also do the same to bubble up the errors.Also, since the whole idea is to remove the word
master, we could doSwitchToMainas the function name?ah good idea!
@ -43,2 +46,4 @@return nil}func MasterToMain(repo *git.Repository) {I'd really appreciate a unit / integration test for this one! To ensure it works, to avoid regressions etc.
added, I ran out of spoons during the first draft but should've made it a WIP PR and got back to it rather than submitting it without the test ig
@ -45,0 +61,4 @@if err := repo.SetConfig(cfg); err != nil {log.Fatalf("Failed to update repository config: %v", err)}fmt.Println("Set 'main' as the default branch.")We normally use
slogto do logging? Do you want to doINFOorDEBUGhere? You'll see it used elsewhere instead offmt.Println.Also, all logging uses lowercase AFAIR, so it's good to follow this convention.
rip, copy-pasting code then forgetting to clean up is embarrassing lol
new recipe default branch main instead of masterto WIP: new recipe default branch main instead of masterWIP: new recipe default branch main instead of masterto new recipe default branch main instead of mastertysm @ammaratef45! LGTM.
I would say one more thing tho. It's up to you, as always.
I am mostly not in favour of keeping these sort of "description of what is happening" comments in the code. And especially on this kind of project which has few "regularly active" maintainers. Things can slip out of understanding very easily and also, things change hands when new people start hacking. That's all good but also, it has disadvantages for code documentation.
The danger is that the comment doesn't represent the code later on when things get changed and nobody feels ownership on the comment or forgets to change it. The comment then misleads future maintainers. I understand this is a bit flame war and personal taste territory but I've already seen comments from long gone maintainers mislead others in this project 🙃
In general, I think a good top-level "doc string" on the function should be enough. It describes "why" and not "what" which is more future proof because you only describe the core of what the code wants to do and this has a greater chance to endure in a more general way and not mislead.
Again, I'm open to argumentation or a decision otherwise but now you know 😌
That's a fair concern, and if logs, errors, and variable and function names are readable enough those comments become redundant anyways 🤷♀️
@ammaratef45 Nice! Cool, well, feel free to merge whenever 😌
The only other point is that
gofmtcomplains about that style of doc string on the function. If you look at the rest of the functions, you'll see we follow the$NAME ...style. But it's fine, just something to keep in mind in the future.The CI failure for
--- FAIL: TestEnsureDomainsResolveSameIPv4seems to be related to coop-cloud/abra#448 on a quick glance but I don't have time to dig into it.