Add support for using directive in java files #639
Conversation
ghostbuster91
force-pushed
the
fix/i533
branch
from
ae7c059
to
8558052
Compare
| val content = value(PreprocessingUtil.maybeRead(j.path)) | ||
| val scopePath = ScopePath.fromPath(j.path) | ||
| val ExtractedDirectives(_, directives0) = | ||
| value(from( |
There was a problem hiding this comment.
It would be nice if we could enrich the DirectiveError here with the information that it happened in the java context but the DirectiveError is an opaque class.
modules/build/src/test/scala/scala/build/tests/SourcesTests.scala
Outdated
Show resolved
Hide resolved
| logger.diagnostic(msg, positions = Seq(position)) | ||
| } | ||
|
|
||
| val usedDirectives = |
There was a problem hiding this comment.
In a situation when Java file starts with following code:
using foo 1
//> using bar 2
User will get a warning that bar is ignored and error that foo is unsupported what is a bit confusing.
There was a problem hiding this comment.
Would you like to resolve priority only against supported directives?
| if (supportedDirectives.contains(usedDirectives.getKind())) | ||
| Right(ExtractedDirectives(offset, strictDirectives)) | ||
| else | ||
| Left(new DirectiveErrors(::(s"Unsupported using directive kind ${usedDirectives.getKind}", Nil))) |
There was a problem hiding this comment.
The message may be confusing since in general keyword using directives are supported but not in the context of Java files and message should reflect that.
There was a problem hiding this comment.
Also, I've just realized that we do not provide a location where are the problematic cases. I think we should use something similar UnusedDirectiveError
There was a problem hiding this comment.
The message may be confusing since in general keyword using directives are supported but not in the context of Java files and message should reflect that.
Agree.
The problem is that higher layers cannot refine error messages from lower layers as they are opaque in the current error design. Here we call same function ExtractedDirectives.from from two different context: scala and java. Each of these context should enrich the message. If only DirectiveErrors was a case class...
I can change that but I am afraid that it will makes this PR much bigger.
|
@ghostbuster91 we are cutting a release and we needed #618 that introduce conflicts with your PR. I am happy to fix the merge conflicts for you, just let me know if I should do it. |
Feel free, I don't mind doing it either way. |
ghostbuster91
force-pushed
the
fix/i533
branch
from
06f9bcf
to
624b75e
Compare
|
When I tried running |
Co-authored-by: Krzysztof Romanowski <romanowski.kr@gmail.com>
ghostbuster91
force-pushed
the
fix/i533
branch
from
5214562
to
eb215fe
Compare
This closes #533
Joint work with @mtk, @romanowski in the framework of the issue spree.