toolshed/abra
4
4
Fork 23
Code Issues 82 Pull Requests 12 Packages Releases 45 Activity

new recipe default branch main instead of master #451

Merged
decentral1se merged 4 commits from defaultBranch into main 2024-12-21 18:11:13 +00:00
Conversation 4 Commits 4 Files Changed 2
+60 -1
ammaratef45 commented 2024-12-13 03:09:51 +00:00
Member
Copy Link
Copy Source

Just saving my future self the trouble of searching "how to rename git branch" every-time I create a new recipe

Just saving my future self the trouble of searching "how to rename git branch" every-time I create a new recipe
ammaratef45 added 1 commit 2024-12-13 03:09:52 +00:00
new recipe default branch main instead of master
continuous-integration/drone/push Build is failing
Details
continuous-integration/drone/pr Build is failing
Details
9510c04aeb
decentral1se reviewed 2024-12-14 21:19:04 +00:00
decentral1se left a comment
Owner
Copy Link
Copy Source

tysm for dipping into this, definitely important work!

tysm for dipping into this, definitely important work!
pkg/git/init.go Outdated
@@ -14,2 +15,4 @@
if err != nil {
return fmt.Errorf("git init: %s", err)
}
MasterToMain(repo)
decentral1se commented 2024-12-14 21:16:27 +00:00
Owner
Copy Link
Copy Source

Since Init returns an error, MasterToMain should also do the same to bubble up the errors.

Since `Init` returns an `error`, `MasterToMain` should also do the same to bubble up the errors.
decentral1se commented 2024-12-14 21:17:08 +00:00
Owner
Copy Link
Copy Source

Also, since the whole idea is to remove the word master, we could do SwitchToMain as the function name?

Also, since the whole idea is to remove the word `master`, we could do `SwitchToMain` as the function name?
ammaratef45 commented 2024-12-15 02:19:38 +00:00
Author
Member
Copy Link
Copy Source

ah good idea!

ah good idea!
ammaratef45 marked this conversation as resolved
pkg/git/init.go Outdated
@@ -43,2 +46,4 @@
return nil
}
func MasterToMain(repo *git.Repository) {
decentral1se commented 2024-12-14 21:17:41 +00:00
Owner
Copy Link
Copy Source

I'd really appreciate a unit / integration test for this one! To ensure it works, to avoid regressions etc.

I'd really appreciate a unit / integration test for this one! To ensure it works, to avoid regressions etc.
ammaratef45 commented 2024-12-15 02:43:00 +00:00
Author
Member
Copy Link
Copy Source

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

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
ammaratef45 marked this conversation as resolved
pkg/git/init.go Outdated
@@ -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.")
decentral1se commented 2024-12-14 21:18:29 +00:00
Owner
Copy Link
Copy Source

We normally use slog to do logging? Do you want to do INFO or DEBUG here? You'll see it used elsewhere instead of fmt.Println.

Also, all logging uses lowercase AFAIR, so it's good to follow this convention.

We normally use `slog` to do logging? Do you want to do `INFO` or `DEBUG` here? You'll see it used elsewhere instead of `fmt.Println`. Also, all logging uses lowercase AFAIR, so it's good to follow this convention.
ammaratef45 commented 2024-12-15 02:28:32 +00:00
Author
Member
Copy Link
Copy Source

rip, copy-pasting code then forgetting to clean up is embarrassing lol

rip, copy-pasting code then forgetting to clean up is embarrassing lol
ammaratef45 marked this conversation as resolved
ammaratef45 added 1 commit 2024-12-15 02:26:33 +00:00
bubble up errors on branch switch
continuous-integration/drone/push Build is failing
Details
continuous-integration/drone/pr Build is failing
Details
8d076a308a
ammaratef45 marked the pull request as work in progress 2024-12-15 02:28:47 +00:00
ammaratef45 added 1 commit 2024-12-15 02:41:41 +00:00
add test for SwitchToMain
continuous-integration/drone/push Build is failing
Details
continuous-integration/drone/pr Build is failing
Details
730fef09a3
ammaratef45 marked the pull request as ready for review 2024-12-15 02:43:20 +00:00
decentral1se reviewed 2024-12-15 14:37:18 +00:00
decentral1se left a comment
Owner
Copy Link
Copy Source

tysm @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 😌

tysm @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 😌
ammaratef45 added 1 commit 2024-12-15 17:53:39 +00:00
replace code-descriptive comments with method level comments
continuous-integration/drone/pr Build is failing
Details
continuous-integration/drone/push Build is failing
Details
28c7676413
ammaratef45 commented 2024-12-15 17:54:39 +00:00
Author
Member
Copy Link
Copy Source

That's a fair concern, and if logs, errors, and variable and function names are readable enough those comments become redundant anyways 🤷‍♀️

That's a fair concern, and if logs, errors, and variable and function names are readable enough those comments become redundant anyways 🤷‍♀️
decentral1se commented 2024-12-15 18:33:46 +00:00
Owner
Copy Link
Copy Source

@ammaratef45 Nice! Cool, well, feel free to merge whenever 😌

The only other point is that gofmt complains 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: TestEnsureDomainsResolveSameIPv4 seems to be related to coop-cloud/abra#448 on a quick glance but I don't have time to dig into it.

@ammaratef45 Nice! Cool, well, feel free to merge whenever 😌 The only other point is that `gofmt` complains 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: TestEnsureDomainsResolveSameIPv4 ` seems to be related to https://git.coopcloud.tech/coop-cloud/abra/pulls/448 on a quick glance but I don't have time to dig into it.
decentral1se merged commit 28c7676413 into main 2024-12-21 18:11:13 +00:00
decentral1se deleted branch defaultBranch 2024-12-21 18:11:13 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
decentral1se
Labels
Clear labels
bug
Something is not working
 build
go build related issues
 ci/cd
Building things with CI/CD
 critical fix
https://docs.coopcloud.tech/federation/resolutions/passed/010/
 design
UI/UX
 documentation
Documenting all the things
 duplicate
This issue or pull request already exists
 easy-first-issue
Something for new people to get stuck into. We hope it's easy!
 enhancement
New feature
 help wanted
Need some help
 i10n
Everything to do with localisation
 i18n
Everything to do with internationalisation
 installer
Everything to do with the install script.
 invalid
Something is wrong
 question
More information is needed
 release
Release management
 release-candidate
Related to the new release candidate
 security
Security related
 tech-debt
test
Unit/integration testing
 wontfix
This won't be fixed
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
3wordchant aadil (Aadil Ayub) abra-bot (Abra Bot) ammaratef45 amras (Sarma) Apfelwurm BornDeleuze Brooke carla cas (Cassowary) coopcloud cyrnel decentral1se (d1) dede devydave fauno (fauno) iexos jade (Jade Ambrose) jjsfunhouse jmakdah2 (Jackie Makdah) joe-irving (Joe Irving) kawaiipunk (KawaiiPunk) knoflook kolaente lambdabundesverband linnealovespie (April) moosemower moritz notplants oxaliq (sorrel) p4u1 pharaohgraphy (Andrew 🐦‍🔥❤️‍🔥✴️) renovate-bot (Comrade Renovate Bot) ripclap simon sixsmith (Sixsmith) stevensting trav (Trav Fryer) val (val (he/him)) yksflip
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: toolshed/abra#451