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.

Sign up for GitHub

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

Jump to bottom

Dart support for trailing commas after named args (WIP). #520

Closed
wants to merge 6 commits into from
Closed

Dart support for trailing commas after named args (WIP). #520

wants to merge 6 commits into from

Conversation

pq
Copy link
Contributor

@pq pq commented Apr 24, 2017

Adds:

screen shot 2017-04-24 at 4 16 17 pm

Besides wordsmithing, the test needs work as this bit in DartServerCompletionContributor

final DartComponent declaration = DartResolveUtil.findConstructorDeclaration(newExpression);

yields null (whereas in practice it does not). Do I need to provoke a resolve?

@alexander-doroshko

@pq pq mentioned this pull request Apr 24, 2017
Closed
@alexander-doroshko
Copy link
Member

I see that similar options for other languages are in the Smart Keys page (where you've already added Insert default argument values in completions). Not an ideal place, but looks more consistent with other technologies and definitely better than 'Code Generation'. This option is about code completion, not about code style/code generation.

Are you saying it doesn't work yet? No guarantees that findConstructorDeclaration() has no bugs or that it is a suitable method for this task. So what should be done before thorough review and landing?

P.S. Please rebase on master.

P.P.S. Do you have more background than mentioned in flutter/flutter-intellij#551? It is marked as a blocker there, but without knowing the background it looks just like a minor questionable feature, which has not been asked for years and has not been implemented in other similar languages. It's about typing a comma... how can it be blocking anything ;-)?
BTW does it save keystrokes? Looks like you'll need to press Right Arrow instead of Comma key. Or is the purpose of the feature some other than saving keystrokes?
From the point of view of this feature what is the difference between named args and standard / positional args?

@pq
Copy link
Contributor Author

pq commented Apr 25, 2017

Thanks for the feedback.

cc @mit-mit for background and justification for the blocker tag.

As for where options should live, I'll migrate. (Aside: the fact that I forgot where the other options were is either testament to my spaciness or the fact that it's not obvious where these things should go! 😉 )

Why would findConstructorDeclaration() not be suitable?

Any thoughts about getting resolution working in the tests?

@alexander-doroshko
Copy link
Member

Why would findConstructorDeclaration() not be suitable?

I'm no answering yet, I'm asking :). I didn't dig into the code yet. So it it suitable? ;)

Any thoughts about getting resolution working in the tests?

In DartServerCompletionTest.java it should work the same as in real run-time, this test uses real analysis server.

@pq
Copy link
Contributor Author

pq commented Apr 25, 2017

I'm no answering yet, I'm asking :). I didn't dig into the code yet. So it it suitable? ;)

Well, I thought so and assumed the code was vetted since it was in the plugin.

In DartServerCompletionTest.java it should work the same as in real run-time, this test uses real analysis server.

Right but the issue is not in what I get from the server, it's in the PSI model. Specifically,

DartResolveUtil.findConstructorDeclaration(newExpression);

returning null in the test but not "in the wild".

Maybe this speaks to the unsuitability of the method? Since I don't know my way around the PSI element model, I could use some guidance on that...

@pq
Merge remote-tracking branch 'upstream/master' into trailing_commas
c0c4f2d 
# Conflicts:
#	Dart/src/com/jetbrains/lang/dart/ide/completion/DartServerCompletionContributor.java
@alexander-doroshko
Copy link
Member

You may see PSI using 'View PSI Structure' action. And get general PSI tree idea looking in dart.bnf file.

@pq
Copy link
Contributor Author

pq commented Apr 25, 2017

Thanks! That plugin was really useful (and the reason I got this far! 👍 )

@alexander-doroshko
Copy link
Member

I'll be able to help tomorrow if anything remains not clear. Feel free to prepare questions.

@alexander-doroshko
Copy link
Member

And still curious about the rationale :).

@pq
Copy link
Contributor Author

pq commented Apr 25, 2017

Updated to a smart key insert preference.

screen shot 2017-04-25 at 3 10 20 pm

@pq
Copy link
Contributor Author

pq commented Apr 25, 2017

More insight into the failing test.

screen shot 2017-04-25 at 3 30 10 pm

Any idea why the REFERENCE_EXPRESSION is resolving to null in com.jetbrains.dart.analysisServer.DartServerCompletionTest#testTrailingCommas?

alexander-doroshko
alexander-doroshko reviewed Apr 26, 2017
View reviewed changes
...src/com/jetbrains/lang/dart/ide/application/options/DartCodeStyleGenerationConfigurable.form Outdated
<properties/>
<border type="etched" title="Auto-insert trailing commas"/>
<children>
<component id="da9d2" class="javax.swing.JCheckBox" binding="myInsertTrailingCommasInConsCheckBox">
Copy link
Member

Choose a reason for hiding this comment

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

why is this file touched?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Forgot to revert. Will fix.

alexander-doroshko
alexander-doroshko reviewed Apr 26, 2017
View reviewed changes
...src/com/jetbrains/lang/dart/ide/application/options/DartCodeStyleGenerationConfigurable.java Outdated
@@ -55,6 +57,7 @@ public String getHelpTopic() {

public void reset(CodeStyleSettings settings) {
myInsertOverrideAnnotationCheckBox.setSelected(settings.INSERT_OVERRIDE_ANNOTATION);
myInsertTrailingCommasInConsCheckBox.setSelected(DartCodeInsightSettings.getInstance().TRAILING_COMMAS_AFTER_CONS_ARGS);
Copy link
Member

Choose a reason for hiding this comment

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

and this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here!

alexander-doroshko
alexander-doroshko reviewed Apr 26, 2017
View reviewed changes
Dart/src/com/jetbrains/lang/dart/ide/formatter/settings/DartCodeStyleSettingsProvider.java Outdated
@@ -9,6 +9,7 @@
import org.jetbrains.annotations.NotNull;

public class DartCodeStyleSettingsProvider extends CodeStyleSettingsProvider {

Copy link
Member

Choose a reason for hiding this comment

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

this empty line finally appears to be the only change in this file :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

@pq
Copy link
Contributor Author

pq commented Apr 26, 2017

Hey @alexander-doroshko. Sorry for the noise. I only partly backed out my changes when I moved to a smart key preference. Should be a little tidier now. 👍

@pq
Copy link
Contributor Author

pq commented Apr 26, 2017

Closing in favor of pushing into server as per conversation in flutter/flutter-intellij#551.

@pq pq closed this Apr 26, 2017
@alexander-doroshko alexander-doroshko self-assigned this Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-doroshko alexander-doroshko alexander-doroshko left review comments

Assignees

@alexander-doroshko alexander-doroshko

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@pq @alexander-doroshko