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

Fix typo in CodeMiningFragment #3226

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

awadammar
Copy link

Introduces a minor correction in the CodeMiningFragment class.

@cdietrich
Copy link
Member

Hi, can you check the status of your eclipse contributor agreement https://www.eclipse.org/legal/eca/

there also seem to be encoding changes in the pr.
Please also commit the Java file generated from this Xtend file

addConfiguredBinding("CodeMinding", '''
binder.bind(�'org.eclipse.jface.text.codemining.ICodeMiningProvider'.typeRef�.class)
.to(�codeMiningProviderClass�.class);
binder.bind(�'org.eclipse.xtext.ui.editor.reconciler.IReconcileStrategyFactory'.typeRef�.class).annotatedWith(�Names.typeRef�.named("codeMinding"))
Copy link
Member

Choose a reason for hiding this comment

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

hmmm wont this break clients? did you check if this is used somewhere

Copy link
Member

Choose a reason for hiding this comment

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

@szarnekow wdyt? break and write something in release notes?

Copy link
Author

Choose a reason for hiding this comment

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

By doing a simple text search, It's only referenced in AbstractDomainmodelUiModule#configureCodeMinding and AbstractArithmeticsUiModule#configureCodeMinding which don't seem to be used

Copy link
Contributor

Choose a reason for hiding this comment

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

If we consider only this generated binding in src-gen a regeneration will fix things in the generated code.
If one had customized such binding, they should get an error if they had used @Override.
In general, I'd be in favor of this change, of course, documenting it.

Copy link
Member

Choose a reason for hiding this comment

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

@LorenzoBettini i meant on user side. @Inject @Named("...")

Copy link
Contributor

Choose a reason for hiding this comment

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

@cdietrich you're right!
However, it would be great if we could get rid of the typo ;)

Copy link
Member

Choose a reason for hiding this comment

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

yes i assume nobody is using it. at least in xtext codebase we seem not to

@awadammar awadammar force-pushed the fix_typo_in_code_mining_fragment branch from 89a4f25 to 3b41c69 Compare October 16, 2024 04:41
@awadammar awadammar force-pushed the fix_typo_in_code_mining_fragment branch from 3b41c69 to dfc2175 Compare October 16, 2024 04:57
@LorenzoBettini
Copy link
Contributor

So, if I understand correctly, what's left to be done is

  • ensure the encoding is correct
  • regenerate the languages that were using that binding (at least, AbstractXXXUiModule)

@cdietrich
Copy link
Member

Yes

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.

3 participants