Skip to content

Conversation

@xperiandri
Copy link
Collaborator

  • disable FL0046 for Identifiers.fs
  • make constants instead of let bindings in SourceLength.fs

* disable `FL0046` for `Identifiers.fs`
* make constants instead of let bindings in `SourceLength.fs`
Copilot AI review requested due to automatic review settings July 4, 2025 17:39

This comment was marked as outdated.

@xperiandri
Copy link
Collaborator Author

@Numpsy could you review?

@xperiandri xperiandri requested a review from Copilot July 4, 2025 17:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors linter rule code by disabling a specific lint rule, converting various test thresholds into compile-time constants, and modernizing string formatting in the build script.

  • Add [<Literal>] to test threshold definitions in SourceLength.fs
  • Disable FL0046 and replace sprintf with string interpolation in Identifiers.fs
  • Update build.fsx to use interpolation and DotNet.nugetPush, remove Paket

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs Convert test length thresholds to [<Literal>] constants
src/FSharpLint.Core/Rules/Identifiers.fs Disable FL0046 and switch sprintf to string interpolation
src/FSharpLint.Core/Framework/Ast.fs Adjust pattern match for SynExpr.Typar
build.fsx Remove Paket ref, switch to interpolation, replace NuGet push
Comments suppressed due to low confidence (4)

tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs:232

  • The TestMaxLinesInMember fixture is declared but has no assertions; add concrete test cases to verify member-line limits.
    // TODO: Add tests.

tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs:307

  • The TestMaxLinesInUnion fixture is missing actual test assertions; include tests to validate union-line constraints.
    // TODO: Add tests.

tests/FSharpLint.Core.Tests/Rules/Conventions/SourceLength.fs:342

  • The TestMaxLinesInEnum fixture has no implemented tests; consider adding assertions to cover enum-line length behavior.
    // TODO: Add tests.

src/FSharpLint.Core/Framework/Ast.fs:373

  • If SynExpr.Typar actually has two parameters, collapsing to one may miss data; verify the AST shape change is intentional.
        | SynExpr.Typar(_) -> ()

@Numpsy
Copy link
Contributor

Numpsy commented Jul 4, 2025

Looks ok to me (I've been putting the format specifiers in interpolated strings for my own code, I don't know if copilot can be configured to not complain about that?)

@Numpsy
Copy link
Contributor

Numpsy commented Jul 4, 2025

I've been having a go at getting the Fornax doc generation working over in Numpsy@686d139, not quite working yet though

@xperiandri xperiandri merged commit ab1c642 into master Jul 4, 2025
8 of 10 checks passed
@xperiandri xperiandri deleted the slef-lint-fix branch July 6, 2025 14:17
@knocte
Copy link
Collaborator

knocte commented Jul 19, 2025

disable FL0046 for Identifiers.fs

You explained the what but not the why. What was the motivation for doing this?

@xperiandri
Copy link
Collaborator Author

Well, someone decided to name the identifiers in pascal case. Which is logical.
But F# does not allow compiling an interpolated string into a literal.
Probably I should have mentioned that

@knocte
Copy link
Collaborator

knocte commented Jul 20, 2025

Probably I should have mentioned that

Or file that as a bug (with a testcase) so that we can take a closer look? I'm still not sure I understand the issue.

@knocte
Copy link
Collaborator

knocte commented Aug 14, 2025

But F# does not allow compiling an interpolated string into a literal.

But what was the motivation for having constants (literals) in the first place?

@xperiandri
Copy link
Collaborator Author

I do not know. Someone did that before me and you in this project

@knocte
Copy link
Collaborator

knocte commented Aug 14, 2025

I'm not referring to previous code, I'm referring to what you have put in this PR description: "make constants instead of let bindings in SourceLength.fs", what is the motivation of making them constants instead of let bindings?

@xperiandri
Copy link
Collaborator Author

Because constants are Pascal case and let bindings are Camel case. But in the code let bindings are Camel case

@knocte
Copy link
Collaborator

knocte commented Aug 16, 2025

It is so hard to decipher what you exactly mean man. Let me see: are you referring to running FSharpLint on itself? Because I just tested removing that fsharp:disable line (see https://github.com/knocte/FSharpLint/commits/wip/removeRuleDisabling/ ) and CI still passes (SelfCheck still green) so I'm still wondering what this change is for.

@Numpsy
Copy link
Contributor

Numpsy commented Aug 17, 2025

The previously seen issue might have been a bug that was fixed by #758? (where FL0046 previously fired for let bindings, and now doesn't)

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.

4 participants