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

[not for merge] Update to fastparse 2 (#510) #541

Closed
wants to merge 6 commits into from

Conversation

raboof
Copy link
Contributor

@raboof raboof commented Jan 31, 2019

Refs #510.

I had to disable the 'native' build targets for now since fastparse 2 has not been released for Scala Native yet, see com-lihaoyi/fastparse#215.

Copy link
Contributor Author

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Commented on 2 changes I was unsure about

val ValueArray: P[TValue] =
P((Index ~ "[" ~/ (PrimitiveValue | MessageValue).rep(0, ",".~/) ~/ "]")).map(TArray.tupled)
def ValueArray[_: P]: P[TValue] =
P((Index ~ "[" ~/ (PrimitiveValue | MessageValue).rep(0, ",") ~/ "]")).map(TArray.tupled)
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'm not sure the .~/ was meaningful here?

@@ -1,48 +1,37 @@
package scalapb.textformat

import fastparse.WhitespaceApi
import fastparse._
import fastparse.ScriptWhitespace._
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'm not entirely sure this correctly corresponds to Basics.whiteSpace

Disabled 'native' targets for now since fastparse 2 has not been released for
Scala Native yet.
As I don't think it is supposed to be part of the public API?
@raboof raboof force-pushed the updateToFastparse2 branch from e977285 to 4b8138d Compare January 31, 2019 18:58
@SethTisue
Copy link
Contributor

this appears to have been merged in some form...?

@raboof
Copy link
Contributor Author

raboof commented Mar 25, 2019

Looks like 👍!

@raboof raboof closed this Mar 25, 2019
@thesamet
Copy link
Contributor

Yes, thanks again for this contribution.

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