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

[xtext generator] Migration of 'QuickfixProviderFragment' #653

Merged
merged 6 commits into from Oct 9, 2015
Merged

[xtext generator] Migration of 'QuickfixProviderFragment' #653

merged 6 commits into from Oct 9, 2015

Conversation

sailingKieler
Copy link
Contributor

Signed-off-by: Christian Schneider christian.schneider@itemis.de

*/
class �grammar.quickfixProviderClass.simpleName� extends �grammar.quickfixProviderSuperClass� {

// @Fix(MyDslValidator::INVALID_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

The '.' notation is preferred also for static member access

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. Didn't pay attention to that.

Christian Schneider added 2 commits October 6, 2015 10:17
Signed-off-by: Christian Schneider <christian.schneider@itemis.de>
…based quickfix provider stub according to review suggestion.

Signed-off-by: Christian Schneider <christian.schneider@itemis.de>
@sailingKieler
Copy link
Contributor Author

I rebased this change to master a couple of minutes ago, and included Sebastian's suggestion. Please review and merge!

*/
class �grammar.quickfixProviderClass.simpleName� extends �grammar.quickfixProviderSuperClass� {

// @Fix(MyDslValidator.INVALID_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use the real validator name in this code snippet or is it not important?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Decided with Miro to use the actual name.
We lazily ignore the case of generating a quickfix provider stub without generating a validator stub. Are you ok with that?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@szarnekow what do you think about the commented imports (line 94-96);
IMO they are obsolete as 'organize imports' does the trick

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, the commented imports do more harm than good.

Christian Schneider added 3 commits October 6, 2015 15:03
…nt one in QuickfixProvider stub template, according to review suggestion

Signed-off-by: Christian Schneider <christian.schneider@itemis.de>
…b template as discussed in the review

Signed-off-by: Christian Schneider <christian.schneider@itemis.de>
…w QuickfixProviderFragment2

Signed-off-by: Christian Schneider <christian.schneider@itemis.de>
instanceClass
).contributeTo(language.eclipsePluginGenModule);

if (!generateStub) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the early exit pattern in long methods. If I add some logic to the end of this method I will wonder why it's not executed.

I'd suggest extracting a method

if (generateStub)
  doGenerateStub

@szarnekow
Copy link
Contributor

I think we should remove the commented imports from other fragments, too, if there are any. I didn't check that, though I remember that the old formatter fragment had those.

…n review

Signed-off-by: Christian Schneider <christian.schneider@itemis.de>
oehme added a commit that referenced this pull request Oct 9, 2015
[xtext generator] Migration of 'QuickfixProviderFragment'
@oehme oehme merged commit 89cd592 into eclipse:master Oct 9, 2015
@sailingKieler sailingKieler deleted the aa/quickfixProvider branch October 26, 2015 08:43
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.

None yet

3 participants