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

Fix record definition using Stroustrup style #2484

Merged
merged 8 commits into from
Sep 8, 2022
Merged

Fix record definition using Stroustrup style #2484

merged 8 commits into from
Sep 8, 2022

Conversation

sheridanchris
Copy link
Contributor

@sheridanchris sheridanchris commented Sep 7, 2022

Adjusted the formatting for record definitions with accessibility modifiers using the Stroustrup style.
This may or may not be a naive approach to solving this problem, any feedback would be much appreciated!
This will fix #2481

I'm not sure what the "correct" format is here. I went with my original example in the issue

type Person = private {
    FirstName: string
    LastName: string
}

Tests whether or not record definitions with accessibility modifiers using the stroustrup style are formatted correctly
Adds test using the internal modifier. Just to be safe.
Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Hello, this is a good start!
Please also add an entry in the changelog.
Thanks

@@ -3412,7 +3412,9 @@ and genMultilineSimpleRecordTypeDefn astContext openingBrace withKeyword ms ao'

and genMultilineSimpleRecordTypeDefnAlignBrackets astContext openingBrace withKeyword ms ao' fs closingBrace =
// the typeName is already printed
opt (indent +> sepNln) ao' genAccess
ifStroustrupElse
(opt (sepNlnWhenWriteBeforeNewlineNotEmpty sepSpace) ao' genAccess)
Copy link
Contributor

Choose a reason for hiding this comment

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

The sepNlnWhenWriteBeforeNewlineNotEmpty will not really play out when there is a comment after the access modifier.

Something like:

type Person = private // foo
    {
        FirstName: string
        LastName: string
    }

would be formatted to

type Person = private // foo
{
    FirstName: string
    LastName: string
}

It is better to use sepSpace in this case and let the comment flow after the { like

type Person = private { // foo
    FirstName: string
    LastName: string
}

This isn't super accurate but good enough for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add a test case that covers this scenario?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like you to change (sepNlnWhenWriteBeforeNewlineNotEmpty sepSpace) with sepSpace here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like you to change (sepNlnWhenWriteBeforeNewlineNotEmpty sepSpace) with sepSpace here.

Right, just asking if you'd like me to add an additional test case or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's fine for now. Adding the test case described above wouldn't strictly be correct.
If you have something like private // foo after formatting that part should in theory be preserved. This is now not the case but the output is acceptable, so I would leave it be.

Copy link
Contributor Author

@sheridanchris sheridanchris Sep 8, 2022

Choose a reason for hiding this comment

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

Alright, I've made the requested changes and removed another redundant test case.

CHANGELOG.md Outdated Show resolved Hide resolved
Previous formatting would result in a compiler error if there was a trailing comment after the accessibility modifier.
This is already a tested case and is therefore redundant.
Copy link
Contributor

@nojaf nojaf left a comment

Choose a reason for hiding this comment

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

Many thanks for this first contribution.
I'll most likely release another beta somewhere tomorrow or over the weekend.

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.

Private record definition error with stroustrup style
2 participants