Remove else statements from if/return blocks.

[stephenwithav]

Also, sort runner.Config, model.Githubcontext fields to be clearer.

[ChristopherHX]

sort runner.Config

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
pkg/runner/runner.go

[codecov]

Codecov Report

Attention: Patch coverage is 70.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 75.03%. Comparing base (5a80a04) to head (b1e6d3b).
Report is 126 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/common/git/git.go	66.66%	4 Missing and 1 partial ⚠️
pkg/runner/run_context.go	80.00%	0 Missing and 1 partial ⚠️

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.
📢 Have feedback on the report? Share it here.

[stephenwithav]

@ChristopherHX Would a PR to your fork earn your approval? What changes could be made to this PR?

[ChristopherHX]

No need for you to patch my fork, it's ok for me

[wolfogre]

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.

[stephenwithav]

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?

[wolfogre]

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
}

[stephenwithav]

I am wondering whether it makes the code more readable for developers. IMO, lexicographical order does not help.

The net.URL example is interesting. I would counter by saying that net.URL is glanceable whereas model.GithubContext and runner.Config are not. It would be nice if those could be deconstructed into smaller structs and then composed into the original struct.

[ChristopherHX]

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...

deconstructed into smaller structs

...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 model.GithubContext this is a painpoint for me.

This context is sent from the GitHub Actions service down to the runner and I prefer map[string]interface{} as new type.

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.

[stephenwithav]

I don't currently think...

deconstructed into smaller structs

...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.

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",
	}
}

[stephenwithav]

Will this be accepted if I remove the sorted fields?

[ChristopherHX]

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

[stephenwithav]

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.

[wolfogre]

from my perspective this would increase a chance of a merge by 400%, but let's hear wolfogres opinion

Yeah, 400%! 😄

[stephenwithav]

The revert is complete along with a cleanup of findGitSlug if you're interested.

[allexiusw]

Suggested change

	if matches := matchesRegex(url,
	regexp.MustCompile(fmt.Sprintf(`^https?://%s/(.+)/(.+?)(?:.git)?$`, githubInstance)),
	regexp.MustCompile(fmt.Sprintf(`%s[:/](.+)/(.+?)(?:.git)?$`, githubInstance)),
	); matches != nil {
	if matches := matchesRegex(url,
	regexp.MustCompile(fmt.Sprintf(`^https?://%s/(.+)/(.+?)(?:.git)?$`, githubInstance)),
	regexp.MustCompile(fmt.Sprintf(`%s[:/](.+)/(.+?)(?:.git)?$`, githubInstance)),
	);
	matches != nil {

[allexiusw]

Can you add unit tests for this function? That would be great.