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

Changed default printing of override members to override from member #743

Merged
merged 8 commits into from
Apr 8, 2020

Conversation

Szer
Copy link
Contributor

@Szer Szer commented Apr 7, 2020

linked to #742

the only meaningful change is in src/Fantomas/CodePrinter.fs
from:
|> Option.defaultValue (!- "member ")
to
|> Option.defaultValue (!- "override ")
updated
also added tweaks to trivia collection from tokens

which is according to spec shouldn't break anything.

Now all dispatch slots emits with override which allows more code to be generated from pure AST (check issue for more info)

This PR doesn't change the behavior of AST formatting when Trivia (origin source code) is presented.

UPDATED
Enhanced trivia collection from tokens so now we could preserve member keyword from sources.

@auduchinok
Copy link
Contributor

Could it check that a member is in an interface implementation and use member there?

@Szer
Copy link
Contributor Author

Szer commented Apr 7, 2020

@auduchinok yep, was able to enhance Trivia collection to grab info from tokens to preserve "member" keyword.

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 @Szer ,

Many thanks for this pull request.
It is really satisfying to see that you took the time to think things through.
And made an implementation that fits in how Fantomas works today.
Everything is covered nicely with unit test and I have nothing but praise for this.

Cheers,
nojaf

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