Remove else statements from if/return blocks. #2494
Conversation
Also, sort runner.Config, model.Githubcontext fields to be clearer.
This is not so nice for my fork that adds fields... don't currently have a script that sorts my fork as well All except this files make me to directly agree |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2494 +/- ##
===========================================
+ Coverage 61.56% 75.03% +13.46%
===========================================
Files 53 62 +9
Lines 9002 10017 +1015
===========================================
+ Hits 5542 7516 +1974
+ Misses 3020 1936 -1084
- Partials 440 565 +125 ☔ View full report in Codecov by Sentry. |
|
@ChristopherHX Would a PR to your fork earn your approval? What changes could be made to this PR? |
There was a problem hiding this comment.
sort runner.Config, model.Githubcontext fields
Please don't do this. Perhaps the current order does need to be adjusted indeed, but sorting the fields in lexicographical order is a bad idea. It will scatter the related fields everywhere, making the code extremely difficult to read.
Would grouping the fields and sorting the grouped fields be an acceptable compromise? |
I am wondering whether it makes the code more readable for developers. IMO, lexicographical order does not help. For example: type URL struct {
Scheme string
Opaque string // encoded opaque data
User *Userinfo // username and password information
Host string // host or host:port (see Hostname and Port methods)
Path string // path (relative paths may omit leading slash)
RawPath string // encoded path hint (see EscapedPath method)
OmitHost bool // do not emit empty host (authority)
ForceQuery bool // append a query ('?') even if RawQuery is empty
RawQuery string // encoded query values, without '?'
Fragment string // fragment for references, without '#'
RawFragment string // encoded fragment hint (see EscapedFragment method)
}
// vs
type URL struct {
ForceQuery bool // append a query ('?') even if RawQuery is empty
Fragment string // fragment for references, without '#'
Host string // host or host:port (see Hostname and Port methods)
OmitHost bool // do not emit empty host (authority)
Opaque string // encoded opaque data
Path string // path (relative paths may omit leading slash)
RawFragment string // encoded fragment hint (see EscapedFragment method)
RawPath string // encoded path hint (see EscapedPath method)
RawQuery string // encoded query values, without '?'
Scheme string
User *Userinfo // username and password information
} |
The |
|
The most problems I have with this structure are merge conflicts due to comment reindenting of the go formatter everything else isn't relevant for me. Which might be solved by adding the comment above the struct member. I don't currently think...
...is a good idea, since this somewhat breaks api or intellisense of top level fields. I sometimes find myself to rewrite struct initialization when doing this, eventually due to linter while it actually should work regardless. I would vote for removing This context is sent from the GitHub Actions service down to the runner and I prefer This model always lacks behind properties and I have shadowed this struct by a typeless one that doesn't eats fields not defined in the model. |
Interesting. I tested it just now and never realized LSP wouldn't autocomplete embedded structs. Thanks! package main
type Name struct {
First string
Last string
}
type EmbeddedName struct {
Name
}
func main() {
embeddedName := EmbeddedName{
First: "Steven",
Last: "Edwards",
}
name := Name{
First: "Steven",
Last: "Edwards",
}
} |
|
Will this be accepted if I remove the sorted fields? |
|
from my perspective this would increase a chance of a merge by 400%, but let's hear wolfogres opinion In the past 2021/22 such kind of PR would have been rejected, because it doesn't really improve act by itself |
Understood. My goal here was just to make the code more readable while I get used to the code base. |
Yeah, 400%! 😄 |
This reverts commit d28ad0e.
|
The revert is complete along with a cleanup of findGitSlug if you're interested. |
Also, sort runner.Config, model.Githubcontext fields to be clearer.