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

Sanitize strings that would lead to uneaven quotes in line #1208

Merged
merged 19 commits into from
Nov 9, 2020

Conversation

WalternativE
Copy link
Contributor

  • Adds two tests based on failing formatting
    behavior having hash directives
  • Documentation of triage session in Fantomas & Friends

- Adds two tests based on failing formatting
  behavior having hash directives
@nojaf
Copy link
Contributor

nojaf commented Oct 29, 2020

@Smaug123 we sorta went with a dirty hack to solve the problem. I'm going to revisit the solution and try and come up with something better ;)

@nojaf nojaf linked an issue Oct 31, 2020 that may be closed by this pull request
3 tasks
@nojaf nojaf marked this pull request as ready for review November 1, 2020 20:20
@nojaf
Copy link
Contributor

nojaf commented Nov 1, 2020

So I've refactored the code a bit and tried to come up with a better way of detecting all the defines inside a raw string.
The algorithm is rather primitive were all characters in the string are processed while being aware in what kind of code block you are currently in (string, multiline code comment)

Lastly, I refactored the code so that the define detection dance is only performed once.

Copy link
Contributor

@Smaug123 Smaug123 left a comment

Choose a reason for hiding this comment

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

I don't really understand this fully, but the shape of it certainly makes me think it looks better. (E.g. the fact that we parse out hash-tokens right at the beginning.)

The state machine is big and scary and I don't really feel like I understand it properly!

.fantomasignore Outdated Show resolved Hide resolved
src/Fantomas.Tests/StringTests.fs Show resolved Hide resolved
src/Fantomas.Tests/TestHelpers.fs Show resolved Hide resolved
src/Fantomas/CodeFormatterImpl.fs Outdated Show resolved Hide resolved
src/Fantomas/CodeFormatterImpl.fs Outdated Show resolved Hide resolved
src/Fantomas/TokenParser.fs Outdated Show resolved Hide resolved
src/Fantomas/TokenParser.fs Show resolved Hide resolved
src/Fantomas/TokenParser.fs Outdated Show resolved Hide resolved
src/Fantomas/TokenParser.fs Outdated Show resolved Hide resolved
src/Fantomas/TokenParser.fs Outdated Show resolved Hide resolved
@nojaf
Copy link
Contributor

nojaf commented Nov 3, 2020

Thanks for the review @Smaug123, some very good remarks! I'll revisit this PR later this week.

@nojaf
Copy link
Contributor

nojaf commented Nov 6, 2020

@Smaug123 I'm ready for round two 😄.

@nojaf nojaf requested a review from Smaug123 November 6, 2020 08:14

let currentLine =
sourceCode
|> Seq.skip skip
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of Seq.skip always terrifies me, because it makes it very easy to write things which are quadratic in complexity; but strings are easy to seek around in, so that's fine.

Copy link
Contributor

@Smaug123 Smaug123 left a comment

Choose a reason for hiding this comment

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

I can't say I'm 100% convinced of its correctness, but I don't see anything obviously wrong, and if no tests are broken then that's good enough for me!

@nojaf
Copy link
Contributor

nojaf commented Nov 9, 2020

Sold!

@nojaf nojaf merged commit 5d5472c into fsprojects:master Nov 9, 2020
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.

Let binding in hash directive disappears
3 participants