glutinum-org / cli

https://glutinum.net/
59 stars 6 forks source link

Add preliminary support for Mapped Types #122

Closed roboz0r closed 2 months ago

roboz0r commented 2 months ago
declare const NODES: readonly ["a", "button"];
type Primitives = {
    [E in (typeof NODES)[number]]: boolean;
};

transforms to

[<AllowNullLiteral>]
[<Interface>]
type Primitives =
    abstract member a: bool with get, set
    abstract member button: bool with get, set

Summary

roboz0r commented 2 months ago

I will also have a look to improve the error message because now with the change introduced in this PR, they are not all on par..

If you're happy with my changes to the error reporting I'll go through and add SyntaxKind.name and __SOURCE_FILE__ into the relevant locations. I was primarily thinking of myself when running the tests and going back and forth between the enum list and trying to work out the file where the error originated.

roboz0r commented 2 months ago

I added SyntaxKind.name in the relevant error messages that I found, I also added __SOURCE_FILE__ to errors that aren't throwing exceptions as the exception would contain source information anyway.

MangelMaxime commented 2 months ago

I was primarily thinking of myself when running the tests and going back and forth between the enum list and trying to work out the file where the error originated.

My way of doing it was by copy / searching the context error across the file. But having the name of the source file, is either because in general editors allow you to fuzzy search by filename in general.

It is just a shame that we can't do something like:

module Log =

    let inline info (message: string) =
        printfn $"%s{__SOURCE_FILE__}: %s{message}"

Where __SOURCE_FILE__ is from the caller thanks to inlining the function

MangelMaxime commented 2 months ago

I also gave a chance to re-think the error reporter:

let generateReaderError
    (fileFolder: string, fileName: string, fileLine: string)
    (errorContext: string)
    (reason: string)
    (node: Ts.Node)
    =
    let sourceFile = node.getSourceFile ()
    let lineAndChar = sourceFile.getLineAndCharacterOfPosition node.pos
    let line = int lineAndChar.line + 1
    let column = int lineAndChar.character + 1

    let errorOrigin =
        let fileFolder =
            let index = fileFolder.IndexOf("src/Glutinum.Converter")
            "./" + fileFolder.Substring(index)

        $"%s{fileFolder}/%s{fileName}(%s{fileLine})"

    $"""%s{errorOrigin}: Error while reading %s{errorContext} from:
%s{sourceFile.fileName}(%d{line},%d{column})

%s{reason}

--- Text ---
%s{node.getFullText ()}
---

--- Parent text ---
%s{node.parent.getFullText ()}
---"""

Example of output:

./src/Glutinum.Converter/Reader/TypeNode.fs(184): Error while reading type node (TypeQuery) from:
/Users/mmangel/Workspaces/Github/glutinum-org/Glutinum.Converter/tests/specs/references/typeof/typeofStrings.d.ts(3,12)

Missing symbol

--- Text ---
typeof NODES
---

--- Parent text ---
 (typeof NODES)
---

This allows to ensure that all errors have the same level of information, because it can be easy to forget to add in {__SOURCE_FILE__}.

I decided to use a relative path for reporting the origin of the error because it seems like for VSCode it allows to click on the link to go the file declaration.

If others IDEs have don't support that, we can do something like:

    let errorOrigin =
        let fileFolder =
        #if DEBUG
            $"%s{fileFolder}"
        #else
            let index = fileFolder.IndexOf("src/Glutinum.Converter")
            "./" + fileFolder.Substring(index)
        #endif

like that in DEBUG we generate a full path and in release we have a relative path. The relative path in release, is because showing my system architecture doesn't matter and add noise for no reason.

What do you think? If you agree on this proposition, I can make the change to the PR

roboz0r commented 2 months ago

Where __SOURCE_FILE__ is from the caller thanks to inlining the function

In .NET land we could use [<CallerFilePath>] but I expect that doesn't work with Fable.

roboz0r commented 2 months ago

re-think the error reporter

proposed error format looks good. Just a note to normalize \ to / before processing the paths. Unfortunately it seems like the System.IO.Path methods haven't been ported over to Fable.

MangelMaxime commented 2 months ago

Where __SOURCE_FILE__ is from the caller thanks to inlining the function

In .NET land we could use [<CallerFilePath>] but I expect that doesn't work with Fable.

You would be surprised at how many times I have been surprised by Fable. And you can add +1 to that number because [< CallerFilePath>] works 🚀.

Thanks you for the tips, you have now idea for how long I have been looking for something like that 😅 (probably 5 years). It is really useful, when generating documentation with link to the source code.

I just need to rewrite the generateReaderError from a function to a member. I will probably create a Reporter type like that it will be able to host things like error, warning, info, etc. if we need to extends it later.

re-think the error reporter

proposed error format looks good. Just a note to normalize \ to / before processing the paths. Unfortunately it seems like the System.IO.Path methods haven't been ported over to Fable.

Nothing that a good old Replace can't fix 😅

MangelMaxime commented 2 months ago

@roboz0r This PR was starting to be a bit messy with the different changes between the Reader + logger improvements.

I think it is best if you can open a new PR using the latest version of main in which I added the improvements we talked regarding the logger.

roboz0r commented 2 months ago

New PR #124