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

Easy dotnet conventions to verify #141

Open
wants to merge 30 commits into
base: master
Choose a base branch
from

Conversation

Mersho
Copy link
Contributor

@Mersho Mersho commented Aug 31, 2023

Working on #122

Supersedes #139

@Mersho Mersho changed the title Easy dotnet conventions to verify #122 Easy dotnet conventions to verify Aug 31, 2023
@Mersho Mersho force-pushed the verifyDotnetConventions-squashed branch from 199f076 to 50fe215 Compare August 31, 2023 11:33
@knocte
Copy link
Member

knocte commented Aug 31, 2023

@Mersho 2 things:

  • CI is still red, please fix it.
  • When opening a PR that replaces an old one, please put the link of the old PR in the description, e.g. saying "Supersedes #XY", and then go to the old PR and close it (adding a comment that links to the new PR).

@knocte
Copy link
Member

knocte commented Aug 31, 2023

@Mersho I can see you have pushed something different from the previous PR, what did you change? (I can tell by seeing that the commit hashes are not the same)

@Mersho Mersho closed this Aug 31, 2023
@Mersho Mersho reopened this Aug 31, 2023
@Mersho
Copy link
Contributor Author

Mersho commented Aug 31, 2023

@Mersho I can see you have pushed something different from the previous PR, what did you change? (I can tell by seeing that the commit hashes are not the same)

@knocte I'm just re-pushing and amending, nothing has changed

git commit --amend --no-edit

@knocte
Copy link
Member

knocte commented Aug 31, 2023

please put the link of the old PR in the description, e.g. saying "Supersedes #XY"

You didn't do this yet.

@Mersho
Copy link
Contributor Author

Mersho commented Aug 31, 2023

please put the link of the old PR in the description, e.g. saying "Supersedes #XY"

You didn't do this yet.

Done, I've edited the first comment.

@knocte
Copy link
Member

knocte commented Aug 31, 2023

Done, I've edited the first comment.

That's not the first comment, it's the PR description :)

@knocte
Copy link
Member

knocte commented Sep 4, 2023

FileConventions: improvements to Library.fs

Ideally, the changes that this commit brings should be moved to the corresponding commits that introduced those functions, to avoid git-blame pollution.

@Mersho
Copy link
Contributor Author

Mersho commented Sep 4, 2023

FileConventions: improvements to Library.fs

Ideally, the changes that this commit brings should be moved to the corresponding commits that introduced those functions, to avoid git-blame pollution.

That's true, I'd like to squash the commits if it's okay with you

There are also some old commits that need to be squashed
FileConventions(.Test): fix proj file indent
FileConvention: properly naming variables

@knocte
Copy link
Member

knocte commented Sep 4, 2023

I'd like to squash the commits if it's okay with you

That's very ambiguous, what commits do you intend to squash? FYI I don't want you to squash everything into 1 commit.

@Mersho
Copy link
Contributor Author

Mersho commented Sep 4, 2023

I'd like to squash the commits if it's okay with you

That's very ambiguous, what commits do you intend to squash? FYI I don't want you to squash everything into 1 commit.

nvm, it's fine.

@knocte
Copy link
Member

knocte commented Sep 6, 2023

FileConventions: dotnetFileConvention

This commit msg title is very rough, why doesn't it have a verb? It must summarize well what is happening, and this does not.

The new dotnetFileConvention.fsx script detects empty strings,
wrong namespace usage, and incorrect printf methods on
non-console applications.

dotnetFileConvention? You're clearly saying that this script is taking care of various conventions, not just one, so then the name of the script should be in plural, not singular.

@Mersho Mersho force-pushed the verifyDotnetConventions-squashed branch 2 times, most recently from 758ee35 to 69aad0c Compare September 7, 2023 09:00
@Mersho
Copy link
Contributor Author

Mersho commented Sep 7, 2023

FileConventions: dotnetFileConvention

This commit msg title is very rough, why doesn't it have a verb? It must summarize well what is happening, and this does not.

The new dotnetFileConvention.fsx script detects empty strings,
wrong namespace usage, and incorrect printf methods on
non-console applications.

dotnetFileConvention? You're clearly saying that this script is taking care of various conventions, not just one, so then the name of the script should be in plural, not singular.

I've changed the commit message. Thank you for informing me.

@Mersho Mersho force-pushed the verifyDotnetConventions-squashed branch from 217858e to 541a03b Compare September 7, 2023 13:55
Add tests for DefiningEmptyStringsWithDoubleQuotes function.
Implement DefiningEmptyStringsWithDoubleQuotes function.
Add tests for ProjFilesNamingConvention function.
Implement ProjFilesNamingConvention function.
Add tests for NotFollowingNamespaceConvention function.
Implement NotFollowingNamespaceConvention function.
Add more tests for NotFollowingNamespaceConvention fn.
Fix NotFollowingNamespaceConvention function.
Mersho and others added 22 commits September 14, 2023 15:46
Add one more test for NotFollowingNamespaceConvention function.
Function DoesNamespaceInclude doesn't working on *.cs namespace.
We updated fsdk to the latest version becuase the old
version of fsdk does not have `Misc.BetterAssert` and
we use `Misc.BetterAssert()` becuase assert is for
debug mode.
Using RemoveEmptyEntries for split & removing extra parans
& Regex does not require a new keyword & string formatting
has been corrected.
Add tests for NotFollowingConsoleAppConvention() function.
Ensure that projects that aren't console applications don't
have source files with console methods.

Co-authored-by: Parham <[email protected]>
Add tests for NotFollowingConsoleAppConvention function
and we fix ConsoleAppConvention2 test so that project
name does not contradict new tests.
Fix NotFollowingConsoleAppConvention() function.
Add tests for async project, Async.RunSynchronously only
allowed in console applications.
Fix NotFollowingConsoleAppConvention() function.
Add test for NotFollowingNamespaceConvention fn.
A .fs/fsx file might not have a namespace sometimes.
`Contains()` method catches a lot of false-positives and not
suitable for this situation so we used regex.

Co-authored-by: Parham <[email protected]>
Fix `DefiningEmptyStringsWithDoubleQuotes()` function by
using regex insted of `Contains()`.

Co-authored-by: Parham <[email protected]>
Finding printf and console methods in files & removed
printf methods used for debugging purposes & added
file filter to ReturnAllProjectSourceFile.
The newly developed script can detect wrong empty strings,
wrong project directories, incorrect namespace usage,
and incorrect methods in non-console applications.
An assert error message has been added for *.fsx files &
renaming the func.
@Mersho Mersho force-pushed the verifyDotnetConventions-squashed branch from 59590da to 280d634 Compare September 14, 2023 12:17
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.

3 participants