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

Comment on first constructor argument gets removed #1350

Closed
1 of 3 tasks
Evangelink opened this issue Jan 8, 2021 · 5 comments · Fixed by #1365
Closed
1 of 3 tasks

Comment on first constructor argument gets removed #1350

Evangelink opened this issue Jan 8, 2021 · 5 comments · Fixed by #1365

Comments

@Evangelink
Copy link
Contributor

Issue created from fantomas-online

Code

let automationManager =
    new AnnAutomationManager(
        // WPF or D2D rendering engine must be consistent with the ImageViewerAutomationControl used
        RenderingEngine = new AnnWPFRenderingEngine(),
        // Ensure that the objects are bound to the container
        RestrictDesigners = true,
        // Do not select/edit object after they are drawn
        EditObjectAfterDraw = false,
        EndDrawWhenLostFocus = true)

Result

let automationManager =
    new AnnAutomationManager(
        RenderingEngine = new AnnWPFRenderingEngine(),
        // Ensure that the objects are bound to the container
        RestrictDesigners = true,
        // Do not select/edit object after they are drawn
        EditObjectAfterDraw = false,
        EndDrawWhenLostFocus = true
    )

Problem description

Fantomas seems to remove comment placed on top of the first argument to a constructor call.

Extra information

  • The formatted result breaks by code.
  • The formatted result gives compiler warnings.
  • I or my company would be willing to help fix this.

Options

Fantomas Master at 01/05/2021 21:00:18 - d5f4d9f

Default Fantomas configuration

Did you know that you can ignore files when formatting from fantomas-tool or the FAKE targets by using a .fantomasignore file?

@nojaf
Copy link
Contributor

nojaf commented Jan 8, 2021

Hello @Evangelink, thanks for opening a new issue. Much appreciated.
To solve this, the comment (trivia) above SynExpr.Tuple should be printed.
I believe around

+> col sepComma args (genShortExpr astContext)

and
+> col (sepComma +> sepNln) args (genExprLong astContext)

genTriviaFor SynExpr_Tuple rangeOfTuple could print the trivia.

The rangeOfTuple information is current lost, we should add it to

let (|Tuple|_|) =
and then add it to
let (|AppTuple|_|) =

Would you be interested in submitting a PR?

@Evangelink
Copy link
Contributor Author

Yes I am :) I will have a look at this during the week-end and might ping for some support if that's ok.

@nojaf
Copy link
Contributor

nojaf commented Jan 8, 2021

Yeah sure, sounds good.

@Evangelink
Copy link
Contributor Author

@nojaf Apologies for the delay, I had a look yesterday and there is something I don't get.

The rangeOfTuple information is current lost, we should add it to

let (|Tuple|_|) =

What I understand is to change:

| SynExpr.Tuple (false, exprs, _, _) -> Some exprs

into:

| SynExpr.Tuple (false, exprs, _, rangeOfTuple) -> Some(exprs, rangeOfTuple)

and to make it surface on the AppTuple active pattern.

But I don't get if I am supposed to add one more object to the return or if I am supposed to do some merge with some of the range.

Current code:

| App (e, [ (Paren (lpr, Tuple args, rpr)) ]) -> Some(e, lpr, args, rpr)

New code:

| App (e, [ (Paren (lpr, Tuple (args, rangeOfTuple), rpr)) ]) -> Some(e, lpr, args, rpr) // What to do with 'rangeOfTuple'

@Evangelink
Copy link
Contributor Author

Sorry just forgot to post progress, as you can see on the PR I have the start of something but I get the comment set twice before and after the syntax element. Not gonna lie, I haven't understood enough of the code to actually manage to see what's wrong or even how to see what's wrong...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants