Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2939593002:
Capture imports on copy; update imports on paste (Closed)
|
Created:
3 years, 6 months ago by Brian Wilkerson Modified:
3 years, 5 months ago CC:
reviews_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionCapture imports on copy; update imports on paste
R=scheglov@google.com
Committed: https://github.com/dart-lang/sdk/commit/d31ed8690ec616711ced999c6efb21dfa37a2d2e
Patch Set 1 #
Total comments: 9
Patch Set 2 : address comments #Messages
Total messages: 11 (2 generated)
brianwilkerson@google.com changed reviewers: + devoncarew@google.com, scheglov@google.com
This isn't ready to be committed yet, but I thought a discussion about the specifics of the API makes sense at this point.
lgtm https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... File pkg/analysis_server/tool/spec/spec_input.html (right): https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... pkg/analysis_server/tool/spec/spec_input.html:2827: <field name="path"> Is path used to verify that URI is resolved to the same file in the target library as it was in the source one? Should we specify that this is absolute, normalized path? FilePath? https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... pkg/analysis_server/tool/spec/spec_input.html:2836: The URI that should be used when importing the library. Should we specify the the URI is absolute?
https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... File pkg/analysis_server/tool/spec/spec_input.html (right): https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... pkg/analysis_server/tool/spec/spec_input.html:2827: <field name="path"> I was trying to anticipate how the edit request would be implemented. I think that users would prefer that we used the same style of import in the paste operation as they had in the file from which the copy was taken. Therefore, I expect that we'd want to use the URI most of the time. But I think there will be times when the URI is not sufficient and we'll need the file path in order to create a URI. (I might be wrong about that, though.) So while we could use it to verify correctness, I was thinking of it as a back-up in case the URI was not sufficient. As for how we specify this, yes, it should be absolute and normalized, and a FilePath. https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... pkg/analysis_server/tool/spec/spec_input.html:2836: The URI that should be used when importing the library. I think we actually want the URI used in the original source so that we can use the same style of URI for the edit.
https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... File pkg/analysis_server/tool/spec/spec_input.html (right): https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... pkg/analysis_server/tool/spec/spec_input.html:2836: The URI that should be used when importing the library. On 2017/06/13 04:15:06, Brian Wilkerson wrote: > I think we actually want the URI used in the original source so that we can use > the same style of URI for the edit. Hm... If the URI is not absolute, i.e. it is relative, it would not mean the same in a different library. We might want to stick to the URI style in the target library though. If it uses relative URIs, and the import URI is for a file in the lib/ of the same package, we would use relative URI, even if the import was copied as an absolute URI. But in the target library uses absolute URIs, we want do the same. https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... pkg/analysis_server/tool/spec/spec_input.html:2846: </field> I think we might need also the "prefix" field. The copied source might reference top-level elements with prefixes, and we need either make sure that we insert imports with the same prefixes, or we need to rewrite the copied code to not use prefixes. Actually sometimes we might have to do this anyway, e.g. if the prefix conflicts with a prefix in the target library. Alternatively, we could say that we don't support rewrites on paste, so conflicts are not "complete". Or maybe we don't support and make as not "complete" copying imports for code with prefixes.
https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... File pkg/analysis_server/tool/spec/spec_input.html (right): https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... pkg/analysis_server/tool/spec/spec_input.html:2836: The URI that should be used when importing the library. > We might want to stick to the URI style in the target library though. > If it uses relative URIs, and the import URI is for a file in the > lib/ of the same package, we would use relative URI, even if the > import was copied as an absolute URI. But in the target library uses > absolute URIs, we want do the same. Agreed. I think keeping the original URI and the file path is enough to allow us the flexibility to optimize the UX. https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... pkg/analysis_server/tool/spec/spec_input.html:2846: </field> > I think we might need also the "prefix" field. I agree. I'll update the CL. I've taken the other issues off-line because I think they require a longer discussion than is convenient in a review.
https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... File pkg/analysis_server/tool/spec/spec_input.html (right): https://codereview.chromium.org/2939593002/diff/1/pkg/analysis_server/tool/sp... pkg/analysis_server/tool/spec/spec_input.html:2846: </field> I think we might also need to capture whether there was a show combinator, whether the import is deferred, and whether the import was conditional. Perhaps rather than capturing individual pieces we should just capture the text of the whole import directive.
PTAL I've uploaded the changes based on feedback. I'd like to try to get this landed next week before I go on vacation.
LGTM
Description was changed from ========== Capture imports on copy; update imports on paste ========== to ========== Capture imports on copy; update imports on paste R=scheglov@google.com Committed: https://github.com/dart-lang/sdk/commit/d31ed8690ec616711ced999c6efb21dfa37a2d2e ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as d31ed8690ec616711ced999c6efb21dfa37a2d2e (presubmit successful). |
