Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scripts/checkCommits1by1.fsx: augment #97

Merged

Conversation

tehraninasab
Copy link
Contributor

@tehraninasab tehraninasab commented Mar 27, 2023

Improve this fsx script to also detect commits with bad CI
status. That is, if commit msg contains text "failing test"
then it would be allowed to have broken CI, else CI should be
green.

Fixes #95

@knocte
Copy link
Member

knocte commented Mar 27, 2023

Also "Compile F# scripts" step in GitHubCI is
removed because there is no error in
detectNotUsingGitPush1by1.fsx script
but compileFSharpScripts.fsx script can not
compile this file.

WHAT????

First, I don't understand why it would not be able to compile this file. And second, don't you think you should explain why it cannot compile this file? The word why in WorkflowGuidelines.md is referring to the ultimate why. Do not hide information to the next developer.

@knocte
Copy link
Member

knocte commented Mar 27, 2023

First, I don't understand why it would not be able to compile this file. And second, don't you think you should explain why it cannot compile this file? The word why in WorkflowGuidelines.md is referring to the ultimate why. Do not hide information to the next developer.

Third, do you think it would be acceptable that we remove the ability to check the correctness of ALL f# SCRIPTS just because ONE of them cannot be compiled?

@tehraninasab
Copy link
Contributor Author

Also "Compile F# scripts" step in GitHubCI is
removed because there is no error in
detectNotUsingGitPush1by1.fsx script
but compileFSharpScripts.fsx script can not
compile this file.

WHAT????

First, I don't understand why it would not be able to compile this file. And second, don't you think you should explain why it cannot compile this file? The word why in WorkflowGuidelines.md is referring to the ultimate why. Do not hide information to the next developer.

I think it should be investigated and resolved in fsxc. What can I do about it in this PR?

@knocte
Copy link
Member

knocte commented Mar 28, 2023

I think it should be investigated and resolved in fsxc

If you think it's a fsxc bug, then:

  1. State so in the commit message, there's no reason to hide your suspicions.
  2. Or even go file a bug in fsx repo, and then reference the bug URL in your commit message so that the next developer understands why the script cannot be compiled, by reading your commit message.

However, this problem you're having is basically the same we're dealing with in #92 , so fix PR92 first with my guidance there (by doing the thing I initially told you to), and then after you do that, you would be able to apply the same thing here.

By the way when I say by doing the thing I initially told you to I don't intend to sound snarky, but think about it from my point of view: I tell you to do A, then you decide that for doing A you would need to do something that looks a bit "ugly" (putting a very long string in an F# file) so you try to fix the ugliness by trying other technique B slightly different than A: you suffer problems with technique B, and decide to solve the problem with a workaround C that deep down you know would be unacceptable by me (eliminating an entire step from CI?), instead of forgetting about B and really doing A as I asked you to.

@tehraninasab
Copy link
Contributor Author

I think it should be investigated and resolved in fsxc

If you think it's a fsxc bug, then:

  1. State so in the commit message, there's no reason to hide your suspicions.
  2. Or even go file a bug in fsx repo, and then reference the bug URL in your commit message so that the next developer understands why the script cannot be compiled, by reading your commit message.

However, this problem you're having is basically the same we're dealing with in #92 , so fix PR92 first with my guidance there (by doing the thing I initially told you to), and then after you do that, you would be able to apply the same thing here.

By the way when I say by doing the thing I initially told you to I don't intend to sound snarky, but think about it from my point of view: I tell you to do A, then you decide that for doing A you would need to do something that looks a bit "ugly" (putting a very long string in an F# file) so you try to fix the ugliness by trying other technique B slightly different than A: you suffer problems with technique B, and decide to solve the problem with a workaround C that deep down you know would be unacceptable by me (eliminating an entire step from CI?), instead of forgetting about B and really doing A as I asked you to.

I didn't know the compile error is because of ResolutionFolder. I thought when you say the code could be greatly simplified by using FSharp.Data JSON parser instead of regex, it's not ok to put 500 lines of JSON string in the script and it's supposed to become shorter. Sorry.

@tehraninasab tehraninasab force-pushed the improveGitPush1by1Check-squashed branch from fe8758c to 58ed1fb Compare March 28, 2023 09:40
@knocte
Copy link
Member

knocte commented Mar 28, 2023

Please rebase this PR 🙏 Thanks

@knocte
Copy link
Member

knocte commented Mar 28, 2023

I didn't know the compile error is because of ResolutionFolder.

I didn't know either, but you don't need to know; remember what I always say? Make it work, then after that, make it pretty. You were making it pretty (because 500 in-line string is ugly) before making it work.

@tehraninasab tehraninasab force-pushed the improveGitPush1by1Check-squashed branch from 58ed1fb to d0e72c6 Compare March 28, 2023 10:43
@tehraninasab tehraninasab force-pushed the improveGitPush1by1Check-squashed branch 2 times, most recently from 1b8d5f9 to 9e1b973 Compare March 29, 2023 08:42
@tehraninasab tehraninasab force-pushed the improveGitPush1by1Check-squashed branch from 9e1b973 to 37ac729 Compare March 29, 2023 13:12
@knocte
Copy link
Member

knocte commented Apr 12, 2023

Unfortunately there are conflicts, please do a rebase when you have time (low-priority task).

@tehraninasab tehraninasab force-pushed the improveGitPush1by1Check-squashed branch from 37ac729 to c822044 Compare April 17, 2023 13:57
@tehraninasab
Copy link
Contributor Author

Unfortunately there are conflicts, please do a rebase when you have time (low-priority task).

Done

@knocte
Copy link
Member

knocte commented May 23, 2023

@realmarv can you rebase this PR please

@tehraninasab tehraninasab force-pushed the improveGitPush1by1Check-squashed branch 3 times, most recently from e5c8417 to ec102a1 Compare May 25, 2023 08:21
@tehraninasab tehraninasab force-pushed the improveGitPush1by1Check-squashed branch 4 times, most recently from 5b530d4 to 6df7345 Compare May 25, 2023 09:43
@knocte knocte force-pushed the master branch 2 times, most recently from d1bd1c2 to 908c11d Compare June 2, 2023 08:09
@knocte
Copy link
Member

knocte commented Jun 29, 2023

This PR needs to be rebased, when you have time. I've also realised that since the script would gain a new feature (not related to "detect not using gitPush1by1"), then we should rename the script. Maybe analyzeCommits1by1.fsx?

&& not(commitMessage.Contains "failing test")
then
Console.Error.WriteLine
"Thanks for pushing commits 1 by 1, however we have detected some of them to not be successful (or not be red when they add a failing test)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@realmarv nit: we have detected some -> it has been detected that some (as using "we" is a bit weird)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@tehraninasab tehraninasab force-pushed the improveGitPush1by1Check-squashed branch 2 times, most recently from 122ec13 to 0289f54 Compare July 4, 2023 12:08
@tehraninasab
Copy link
Contributor Author

This PR needs to be rebased, when you have time. I've also realised that since the script would gain a new feature (not related to "detect not using gitPush1by1"), then we should rename the script. Maybe analyzeCommits1by1.fsx?

Done

Improve this fsx script to also detect commits with bad CI
status. That is, if commit msg contains text "failing test"
then it would be allowed to have broken CI, else CI should be
green.

Fixes nblockchain#95
@knocte knocte force-pushed the improveGitPush1by1Check-squashed branch from 0289f54 to 753014c Compare July 5, 2023 01:51
@knocte knocte changed the title scripts/detectNotUsingGitPush1by1: improve scripts/checkCommits1by1.fsx: augment Jul 5, 2023
@knocte knocte merged commit ae38002 into nblockchain:master Jul 5, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve scripts/detectNotUsingGitPush1by1.fsx to detect broken(red) CI statuses
2 participants