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

Simplifify api for import patches #48

Merged
merged 2 commits into from
Jan 28, 2017

Conversation

olafurpg
Copy link
Contributor

No description provided.

@olafurpg olafurpg requested a review from ShaneDelmore January 28, 2017 20:30
@@ -0,0 +1,23 @@
// Small worksheet to demonstrate usage of import patches
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ShaneDelmore I got this working on my laptop
screen shot 2017-01-28 at 21 30 54

Copy link
Contributor

@ShaneDelmore ShaneDelmore left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for the usage demonstration.

object RewriteCtx {
def fromCode(ast: Tree,
config: ScalafixConfig = ScalafixConfig(),
semanticApi: Option[SemanticApi] = None): RewriteCtx = {
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if no SemanticApi is supplied? Is a new one created, or is functionality diminished in some way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. It depends on what rewrite is used, some rewrites like procedure syntax work 100% without the semantic api, organize imports works without it but can't handle relative imports in that case, explicit implicit will error.

I guess this could be better documented.

val tokenToEdit =
oldImports.headOption
.map(_.tokens.head)
.getOrElse(tokens.head)
.getOrElse(ctx.tokens.head)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know ctx.tokens is non-empty 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.

The tree nodes that return empty tokens are incredibly few (if any) and I would say it was a truly exceptional case if those were passed in here. Top level trees always have a beginning of file token.

There's an interesting ongoing discussion in scalameta about validating trees for a given dialect. I would be interested to start a similar discussion about the possibility to use a non empty list type Where applicable. The challenge is try and encode as much as possible on the type level without burdening syntax too heavily. Quasi quotes help a lot here.

@ShaneDelmore ShaneDelmore merged commit 69069ff into scalacenter:master Jan 28, 2017
bjaglin pushed a commit to liancheng/scalafix that referenced this pull request May 23, 2023
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.

2 participants