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

Renamed MdocSession.App to MdocSession.MdocApp #655

Merged
merged 1 commit into from
Jun 10, 2022
Merged

Conversation

hnaderi
Copy link
Contributor

@hnaderi hnaderi commented Jun 8, 2022

App is a common name which conflicts with some codes and there is no meaningful workaround I think. so I renamed it to a more descriptive name that is also extremely hard to get ambiguous reference error from.
it might resolve #204 too.

info] Reference to App is ambiguous,
[info] it is both defined in object MdocSession
[info] and inherited subsequently in object Foo

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.

Thank you for this contribution! I am concerned that the name repl.MdocSession$MdocSessionAppBody is unnecessarily verbose given that it appears in all stack traces. I'm OK with changing the name but I'd prefer to use something shorter like ffun (or anything in that direction really). What do you think?

@@ -299,8 +299,8 @@ class WorksheetSuite extends BaseSuite {
compat = Map(
Compat.Scala3 ->
"""|crash:4:1: error: java.lang.RuntimeException: boom
| at repl.MdocSession$App.crash(crash.scala:7)
| at repl.MdocSession$App.<init>(crash.scala:15)
| at repl.MdocSession$MdocSessionAppBody.crash(crash.scala:7)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| at repl.MdocSession$MdocSessionAppBody.crash(crash.scala:7)
| at repl.MdocSession$MdocApp.crash(crash.scala:7)

Maybe? Will be shorter a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! just renamed it to your suggestion.

@hnaderi
Copy link
Contributor Author

hnaderi commented Jun 10, 2022

Thank you for this contribution! I am concerned that the name repl.MdocSession$MdocSessionAppBody is unnecessarily verbose given that it appears in all stack traces. I'm OK with changing the name but I'd prefer to use something shorter like ffun (or anything in that direction really). What do you think?

I considered other more generic names, but in the end I thought it would be simpler and more tolerant to name conflicts if it contains Mdoc in its name.

@hnaderi hnaderi changed the title Renamed MdocSession.App to MdocSessionAppBody Renamed MdocSession.App to MdocSession.MdocApp Jun 10, 2022
@hnaderi hnaderi requested review from olafurpg and tgodzik June 10, 2022 14:23
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@tgodzik tgodzik merged commit b33d8c4 into scalameta:main Jun 10, 2022
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.

it is both defined in object Session and imported subsequently by error
3 participants