Skip to content

[All] Add support for TreatWarningsAsErrors#4073

Merged
MangelMaxime merged 5 commits into
mainfrom
support-treat-warnings-as-errors
Mar 12, 2025
Merged

[All] Add support for TreatWarningsAsErrors#4073
MangelMaxime merged 5 commits into
mainfrom
support-treat-warnings-as-errors

Conversation

@MangelMaxime

@MangelMaxime MangelMaxime commented Mar 12, 2025

Copy link
Copy Markdown
Member

This PR tries to re-enable TreatWarningsAsErrors, this is really handy feature and becoming even more important with nullness supports to make nullness really strict.

We mimic the behavior at Fable level, because we can pass the option to FCS otherwise it will also report errors from libraries/packages code which the user have no control over.

#2521

Comment thread src/Fable.Cli/Main.fs Outdated
@MangelMaxime
MangelMaxime marked this pull request as draft March 12, 2025 10:56
Comment thread src/Fable.Cli/Main.fs
otherLogs
|> Array.iter (fun log ->
match log.Severity with
| Severity.Error -> () // In theory, we shouldn't have errors here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we wanted to avoid this cases, we should need to rework the LogEntry type from a record:

type LogEntry =
    {
        Message: string
        Tag: string
        Severity: Severity
        Range: SourceLocation option
        FileName: string option
    }

into a union

type LogData =
    {
        Message: string
        Tag: string
        Range: SourceLocation option
        FileName: string option
    }
	

type LogEntry = 
	| Error of LogData
	| Warning of LogData
	| Info of LogData

I am not sure if this is worth doing or not

@MangelMaxime
MangelMaxime marked this pull request as ready for review March 12, 2025 16:09
@MangelMaxime

Copy link
Copy Markdown
Member Author

If we wanted to add integration tests for this feature we would need to make a NuGet package with warnings inside of it to check that do ignore them and don't report them as errors.

The previous implementation could have been tested the same way, so for now for simplicity I think we can avoid going down that path for simplicity. If later, we found issues with the implementation or want to prevent regression we can allocate some time to try crafting a test suit for both warnings and errors output of Fable.

@MangelMaxime

Copy link
Copy Markdown
Member Author

@nojaf @Freymaurer @jkone27

I believe you will need to port this code into vite-plugin-fable.

As support for TreatWarningsAsErrors has been tried several times in the past and disabled, if I were you I would wait before porting it to make sure that this implementation is correct and stable.

@Freymaurer

Copy link
Copy Markdown
Contributor

Let me know when! 🙂

Comment thread src/Fable.Cli/Main.fs Outdated
@MangelMaxime

Copy link
Copy Markdown
Member Author

I am going to merge this PR as is, because more "real world testing" is needed to gather feedback.

There are several things that could be improve once we know it works:

  1. Avoid doing the array concatenation at the end of startCompilationAsync
  2. Adapt the integration tests, and we should probably simplify then if possible

@MangelMaxime
MangelMaxime force-pushed the support-treat-warnings-as-errors branch from b31ac70 to dfd5d0f Compare March 12, 2025 21:52
@MangelMaxime
MangelMaxime merged commit 34ca900 into main Mar 12, 2025
@MangelMaxime
MangelMaxime deleted the support-treat-warnings-as-errors branch March 12, 2025 21:52
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