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

#2633 Split PatchDelegate into 2 classes, 1 without jdt.core deps. #2634

Closed
wants to merge 1 commit into from

Conversation

rgra
Copy link
Contributor

@rgra rgra commented Oct 29, 2020

Fixes #2633 by splitting up PatchDelegate into PatchDelegateChildren and delegating to it via an interface

Copy link
Collaborator

@Rawi01 Rawi01 left a comment

Choose a reason for hiding this comment

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

  • There should be a formatter profile if you import lombok in eclipse, can you use that one? Your current profile removed a lot of whitespace.
  • New files should use only 2020 as copyright year.
  • findAlreadyImplementedMethods is not an eclipse only method, disabling this in ecj mode will lead to inconsistent behaviour.
  • I don't think that it is required to split up Reflection into two classes as long as getChildren is the second method in the static block.
  • I don't think that it is required to split everything up into two classes, you can use the non existing class as long as you do not create an instance and do not looup the methods using reflection. It might be easier to move all eclipse only methods into a nested class e.g. EclipseOnlyMethods that detects if eclipse is available and if not all methods simply return null. Example:
private static class EclipseOnlyMethods {
	private static boolean eclipseAvailable = true;
	static {
		try {
			CompilationUnit.class.getName();
		} catch (NoClassDefFoundError e) {
			eclipseAvailable = false;
		}
	}
	private static List<SourceMethod> getDelegateMethods(SourceType sourceType) {
		if (!eclipseAvailable) return null;
		...
	}
	...
}

Disclaimer: I'm not a project owner.

@Rawi01 Rawi01 mentioned this pull request Feb 7, 2021
@rzwitserloot
Copy link
Collaborator

@Rawi01 Given that you wrote in already-merged PR #2742 that that one 'fixes' this one, can I close this without merging it?

@Rawi01
Copy link
Collaborator

Rawi01 commented Jun 3, 2021

@rzwitserloot Yes, this PR can be closed. I missed to do that, sorry for wasting your time.

@Rawi01 Rawi01 closed this Jun 3, 2021
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.

[BUG] Regression: #2006 breaks Delegate in Tycho
3 participants