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

Detect non-verbose flags being used in F# scripts or YML CI files #91

Merged

Conversation

tehraninasab
Copy link
Contributor

No description provided.

@tehraninasab tehraninasab force-pushed the detectNonVerboseFlags-squashed branch 3 times, most recently from da363a9 to daf36a7 Compare March 23, 2023 13:24
@knocte
Copy link
Member

knocte commented Mar 28, 2023

Please rebase this PR 🙏 Thanks

@tehraninasab tehraninasab force-pushed the detectNonVerboseFlags-squashed branch from 2d5c202 to 7e9f86f Compare March 28, 2023 10:25
@tehraninasab
Copy link
Contributor Author

Please rebase this PR pray Thanks

Done

@knocte knocte force-pushed the master branch 2 times, most recently from d1bd1c2 to 908c11d Compare June 2, 2023 08:09
@tehraninasab tehraninasab force-pushed the detectNonVerboseFlags-squashed branch 5 times, most recently from 8e3f57a to 021111d Compare July 4, 2023 12:29
assert
validExtensions
|> Seq.map(fun ext -> fileInfo.FullName.EndsWith(ext))
|> Seq.contains true
Copy link
Member

Choose a reason for hiding this comment

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

@realmarv can you explain what exactly are you doing here please? this is a bit unreadable; also, why are you using assert instead of failwith? I'm actually not familiar with assert func TBH, but I have the feeling that it only works in DEBUG mode and the error thrown would be very unreadable (as opposed to using failwith where you are required to provide a readable error)

Copy link
Member

Choose a reason for hiding this comment

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

@realmarv BTW I just cooked this in case you want to use it: nblockchain/fsx@7acb7a5 (will be available in nuget in 5min)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It loops over validExtensions sequence and checks if the file path ends with any of them

@knocte
Copy link
Member

knocte commented Jul 5, 2023

@realmarv I raised 2 nits before this can be merged; but please before addressing them, please pull, because I added a commit to your branch

@knocte knocte force-pushed the detectNonVerboseFlags-squashed branch from 3e07e1d to 765a2c4 Compare July 5, 2023 08:20
@knocte
Copy link
Member

knocte commented Jul 5, 2023

@realmarv also PR's CI is red

Add tests for NonVerboseFlagsInGitHubCI function.
Implement NonVerboseFlagsInGitHubCI function.
Fix NonVerboseFlagsInGitHubCI function.
Cover .sh and .fs files in NonVerboseFlagsInGitHubCI.
Detect non-verbose flags in .yml, .sh, .fs and .fsx files.
Run nonVerboseFlagsInGitHubCIAndFsharpScripts.fsx script and
convert non-verbose flags to verbose flags.
@tehraninasab tehraninasab force-pushed the detectNonVerboseFlags-squashed branch 3 times, most recently from 199cf64 to 4156fa4 Compare July 5, 2023 13:39
|> Seq.map(fun ext ->
Helpers.GetInvalidFiles
rootDir
("*" + ext)
Copy link
Member

Choose a reason for hiding this comment

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

@realmarv aren't you missing a dot after * here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No ext already has a dot

@tehraninasab tehraninasab force-pushed the detectNonVerboseFlags-squashed branch from 4156fa4 to be256d4 Compare July 6, 2023 14:12
@knocte
Copy link
Member

knocte commented Jul 6, 2023

CI is still red

@tehraninasab
Copy link
Contributor Author

CI is still red

It's green now

@knocte knocte merged commit effd986 into nblockchain:master Jul 6, 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.

2 participants