InternalTrees: require origin dialect in tokens
#3498
Conversation
kitbellew
force-pushed
the
3498
branch
2 times, most recently
from
b73c77f
to
8651f72
Compare
| @@ -15,7 +15,7 @@ import scala.{meta => sm} | |||
| def children: List[Tree] | |||
|
|
|||
| def pos: Position | |||
| def tokens(implicit dialect: Dialect): Tokens | |||
There was a problem hiding this comment.
This could cause issues still, so maybe instead we should just ignore the dialect and deprecate the method? We could have a separate parameterless one.
There was a problem hiding this comment.
meaning, still keep the parameter but throw it origin doesn't have it?
what issues could be caused, btw?
There was a problem hiding this comment.
If we have a library depending on a certain version of Scalameta (for example Ammonite) and someone brings in the newest one then in case someone uses tokens method it will throw since the earlier version of the method doesn't exist.
There was a problem hiding this comment.
but then if we're planning to increment the minor version of scalameta, aren't we allowed to make binary-incompatible changes as long as they are source-compatible? wouldn't that hypothetical library be expected to be updated for newer scalameta? there must be some process to make these changes occasionally.
There was a problem hiding this comment.
Is it source compatible? If the parameter is set explicitly then it will break users code. I also don't think we semantic versioning says anything about source or binary.
there must be some process to make these changes occasionally.
Some libraries just do a lot of major versions, but it's problematic in case of scalameta.
There was a problem hiding this comment.
Interesting enough I can't find anything about scalameta usage in Ammonite anymore, so maybe changing tokens would ok?
Anyway, I am just being super cautious here, might not be worth the trouble to keep tokens.
There was a problem hiding this comment.
for now, did you what you suggested. if you find that dropping the parameter is also ok, we'll include that in a subsequent pr.
didn't understand your point about "not worth the trouble to keep tokens".
There was a problem hiding this comment.
as to passing dialect as parameter: i did this search: https://github.com/search?q=%22.tokens%28%22++language%3AScala+%22scala.meta%22+NOT+is%3Afork&type=code
i saw two or three cases. i guess i need to add the method without that parameter.
There was a problem hiding this comment.
Thanks for checking!
not worth the trouble to keep tokens
Ignore me, I hadn't had too much sleep at that point in time 😅
For now, keep the implicit dialect parameter for backwards-compatibility but ignore it. Throw if no dialect is available through origin. That way, we will not incur hidden syntax-generation and tokenization costs implicitly; for that, we have the `tokenizeFor` method.
That way, we will not incur hidden syntax-generation and tokenization costs implicitly; for that, we have the
tokenizeFormethod.