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

Add option to specify modifier in the worksheet API #492

Merged
merged 1 commit into from
May 11, 2021

Conversation

tgodzik
Copy link
Contributor

@tgodzik tgodzik commented May 4, 2021

Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

The reset-object modifier is specifically implemented for this feature https://scalameta.org/mdoc/docs/modifiers.html#reset-object

The object encoding can cause deadlocks and other difficult-to-debug errors while the class encoding at least fails fast with a recognizeable error message.

Metals can enable the reset-object modifier without changing mdoc.

@tgodzik
Copy link
Contributor Author

tgodzik commented May 4, 2021

The reset-object modifier is specifically implemented for this feature https://scalameta.org/mdoc/docs/modifiers.html#reset-object

The object encoding can cause deadlocks and other difficult-to-debug errors while the class encoding at least fails fast with a recognizeable error message.

Metals can enable the reset-object modifier without changing mdoc.

It seems we always add the Default modifier, so we would need to replace it with ResetObject one. What do you think?

Or alternatively, add an option to specify the modifier to the worksheet API.

@olafurpg
Copy link
Member

olafurpg commented May 4, 2021

Right, it looks like worksheet clients can't add modifiers. It should be an easy change to make this configurable, a good place to look at would be here

val sectionInput = SectionInput(input, ctx)

There's another overload for SectionInput that takes in a modifier.

@tgodzik tgodzik changed the title Wrap code in object instead of a class Add option to specify modifier in the worksheet API May 5, 2021
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

| case class Bar(b: Int) extends AnyVal
|}""".stripMargin,
"",
modifier = Some("reset-object")
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to add multiple modifiers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is! Added a test to demonstrate it.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome

@tgodzik tgodzik marked this pull request as ready for review May 6, 2021 08:32
@tgodzik
Copy link
Contributor Author

tgodzik commented May 6, 2021

Modifier doesn't exists for Scala 3 currently, so we should be fine to wait for #466

@tgodzik tgodzik force-pushed the fix-issues-with-value branch 2 times, most recently from 5056dcb to c6807f1 Compare May 11, 2021 09:03
@tgodzik tgodzik merged commit 958ce08 into scalameta:main May 11, 2021
@tgodzik tgodzik deleted the fix-issues-with-value branch May 11, 2021 09:18
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.

None yet

2 participants