Refactor + Tidy up code #48

Merged
fnetX merged 11 commits from :refactor-code into hackathon 2 months ago
Gusted commented 2 months ago
Collaborator
  • Add test for mail parsing.
  • Move CompileRegex outside function.
  • Uncapitilized error.
  • Simplify validator checking.
  • Simplify fmt.Println(fmt.Sprintf(...)) into fmt.Printf(...\n).
  • Mark a unused argument in function.
  • Fix the error flow in startRegrestation(return on error).
  • Wrap http.ListenAndServer into fmt.Printf, to log it's error.
- Add test for mail parsing. - Move CompileRegex outside function. - Uncapitilized error. - Simplify validator checking. - Simplify `fmt.Println(fmt.Sprintf(...))` into `fmt.Printf(...\n)`. - Mark a unused argument in function. - Fix the error flow in startRegrestation(return on error). - Wrap http.ListenAndServer into `fmt.Printf`, to log it's error.
Gusted added 8 commits 2 months ago
Gusted added 1 commit 2 months ago
282e85f1dc
Print panic error
Poster
Collaborator

Hmm interesting, not sure why the CI is failing.

Hmm interesting, not sure why the CI is failing.
fnetX reviewed 2 months ago
fnetX left a comment
Collaborator

I'm not fully in this project atm, but please see my comments.

There is currently no big plan for this code, so as long as everything stays functional, I suppose this is fine to merge.

I'm not fully in this project atm, but please see my comments. There is currently no big plan for this code, so as long as everything stays functional, I suppose this is fine to merge.
main.go Outdated
"Options": func(input ...string) []string {
return input
},
"Raw": func(input string) template.HTML {
fnetX commented 2 months ago
Collaborator

is Raw not useful in the future?

is Raw not useful in the future?
Poster
Collaborator

Well... currently it has no use-case 🤷.

Well... currently it has no use-case 🤷.
fnetX marked this conversation as resolved
formData, errors := validate(serverCtx, req.PostForm)
if len(errors) != 0 {
renderCtx = make(renderingContext)
renderCtx := make(renderingContext)
fnetX commented 2 months ago
Collaborator

why is it recreated anyway?

why is it recreated anyway?
Poster
Collaborator

No idea, I assume to remove existing context that is passed along, (not fill it back in?) to just render the error.

No idea, I assume to remove existing context that is passed along, (not fill it back in?) to just render the error.
fnetX marked this conversation as resolved
Collaborator

Regarding CI: Which shell does Woodpecker run with? Is it maybe not bash and thus not properly parsing the brackets / logic?

Regarding CI: Which shell does Woodpecker run with? Is it maybe not bash and thus not properly parsing the brackets / logic?
Collaborator

The issue is with the last line in the current .woodpecker.yml.
It has got nothing to do with the choice of shell. You can easily test it in your own shell by replacing $(gofumpt -extra -l . | wc -l) with echo 0 or echo 1 respectively.

The issue lies in the exit/return code evaluated by the shell:

The current last line reads like this:

- "[ $(gofumpt -extra -l . | wc -l) != 0 ] && { echo 'code not formated'; exit 1; }"

The output of the following is "0":

$(gofumpt -extra -l . | wc -l)

Thus the test

[ $(gofumpt -extra -l . | wc -l) != 0 ]

evaluates to:

[ 0 != 0 ]

which then evaluates to an exit code of 1 by the test-command (which is interpreted as false by the shell in the &&). Try [ 0 != 0 ];echo $? in a CLI of a shell.

The rest of the line is not interpreted, as the and (&&) operator cannot become true anymore.

The return/exit code of the complete line is 1.

The pipeline ends with a failure as the exit code is 1.


Proposed fix:

Use = and || instead:

- "[ $(gofumpt -extra -l . | wc -l) = 0 ] || { echo 'code not formated'; exit 1 }"

If the output of wc -l is 0, this would lead to an exit code of 0 for the test. The result of the || would be true already. Thus the rest of the line is not interpreted anymore.

If the output of wc -l is 1, that would lead to the exit code of 1 for the test. This is interpreted as false in the following ||. The expression can only become true if the next command evaluates to true. Thus it is executed and leads to an exit code of 1 (because of the call to "exit 1" - btw. you could also call "false" to achieve the result). Either way the result code would be 1 and this would fail the CI-test.

The issue is with the last line in the current .woodpecker.yml. It has got nothing to do with the choice of shell. You can easily test it in your own shell by replacing `$(gofumpt -extra -l . | wc -l)` with `echo 0` or `echo 1` respectively. The issue lies in the exit/return code evaluated by the shell: The current last line reads like this: ``` - "[ $(gofumpt -extra -l . | wc -l) != 0 ] && { echo 'code not formated'; exit 1; }" ``` The output of the following is "0": ``` $(gofumpt -extra -l . | wc -l) ``` Thus the test ``` [ $(gofumpt -extra -l . | wc -l) != 0 ] ``` evaluates to: ``` [ 0 != 0 ] ``` which then evaluates to an exit code of 1 by the test-command (which is interpreted as false by the shell in the &&). Try `[ 0 != 0 ];echo $?` in a CLI of a shell. The rest of the line is not interpreted, as the and (&&) operator cannot become true anymore. The return/exit code of the complete line is 1. The pipeline ends with a failure as the exit code is 1. ----- **Proposed fix:** Use = and || instead: ``` - "[ $(gofumpt -extra -l . | wc -l) = 0 ] || { echo 'code not formated'; exit 1 }" ``` If the output of `wc -l` is 0, this would lead to an exit code of 0 for the test. The result of the || would be true already. Thus the rest of the line is not interpreted anymore. If the output of `wc -l` is 1, that would lead to the exit code of 1 for the test. This is interpreted as false in the following ||. The expression can only become true if the next command evaluates to true. Thus it is executed and leads to an exit code of 1 (because of the call to "exit 1" - btw. you could also call "false" to achieve the result). Either way the result code would be 1 and this would fail the CI-test.
Gusted added 1 commit 2 months ago
9aa5a9dfff
Fix CI?
Poster
Collaborator

Interesting behavior, not sure how it was passing the CI before(if it ever did). I've added the proposed fix to the woodpecker.yml

Interesting behavior, not sure how it was passing the CI before(if it ever did). I've added the proposed fix to the woodpecker.yml
Gusted added 1 commit 2 months ago
fnetX merged commit 4685eec9b7 into hackathon 2 months ago
fnetX referenced this issue from a commit 2 months ago
Gusted deleted branch refactor-code 1 month ago
The pull request has been merged as 4685eec9b7.
Sign in to join this conversation.
Loading…
There is no content yet.