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

Explore apt #16

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Explore apt #16

wants to merge 6 commits into from

Conversation

uweschaefer
Copy link
Contributor

@uweschaefer uweschaefer commented Jul 20, 2020

Not sure if that makes it better. Certainly the processor is hard to understand and test, but as a client, you only see easy generated code and you can navigate to handle methods easily (which were the initial drivers).

Please comment.

To use this in IIP for instance, just do a local install and update the dependency in the project's pom - nothing more to do.

@codecov
Copy link

codecov bot commented Jul 20, 2020

Codecov Report

Merging #16 into master will decrease coverage by 92.48%.
The diff coverage is 0.00%.

@@              Coverage Diff              @@
##              master     #16       +/-   ##
=============================================
- Coverage     100.00%   7.51%   -92.49%     
+ Complexity        20       6       -14     
=============================================
  Files              9      12        +3     
  Lines             89     266      +177     
  Branches           4      25       +21     
=============================================
- Hits              89      20       -69     
- Misses             0     246      +246     

@uweschaefer uweschaefer requested a review from samba2 July 20, 2020 17:48
@samba2
Copy link

samba2 commented Jul 20, 2020

Tnx for asking. Happy to comment. Quite busy at the beginning of this week but will try to squeeze it in.

Copy link

@DanielGronau DanielGronau left a comment

Choose a reason for hiding this comment

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

The only problem I see is the unclosed InputStream in ManifestHelper, everything else is cosmetics.

}
}
} catch (Exception e) {
}

Choose a reason for hiding this comment

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

I would add an comment that ignoring the catch block is intentional.

}
}
} catch (IOException e1) {
}

Choose a reason for hiding this comment

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

Same here.

int lastDot = className.lastIndexOf('.');
if (lastDot > 0) {
packageName = className.substring(0, lastDot);
}

Choose a reason for hiding this comment

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

Theoretically, packageName could be obtained via (PackageElement)(type.getEnclosingElement()), but I'm not sure this is better here.

while (resEnum.hasMoreElements()) {
try {
URL url = resEnum.nextElement();
InputStream is = url.openStream();

Choose a reason for hiding this comment

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

Looks like is is never closed.

@uweschaefer
Copy link
Contributor Author

@DanielGronau thx for reviewing and pointing out some of the little defects. As i said it is POC level code. However i am interested in your opinion, if this is something to pursue.
Currently i am on the fence. If everything goes to plan, the results of this lib might be easier to reason about than AOP. Also this might help with going native. - However, if something goes wrong here, it is probably harder to debug than AOP and also while there is a good load of documentation about Spring-AOP on the web, this technique is probably more niche.

@DanielGronau
Copy link

Currently i am on the fence. If everything goes to plan, the results of this lib might be easier to reason about than AOP. Also this might help with going native. - However, if something goes wrong here, it is probably harder to debug than AOP and also while there is a good load of documentation about Spring-AOP on the web, this technique is probably more niche.

I agree this it is a difficult decision. The reflection / mirror stuff makes the code hard to read, but I think this could be improved by introducing an abstraction layer. In order to support debugging, having good logging in place could help a lot. The big plus I see - beside the client experience and nativity - is getting more control and freedom. So I would suggest to go forward.

@samba2
Copy link

samba2 commented Jul 22, 2020

Ok, in the last 1/2 h I've learned quite something about apt. This was indeed interesting ;-) But instead of having us reverse engineer: could you please provide an example (or running test) how a user of the library is supposed to use it? Or do I miss something?

@uweschaefer
Copy link
Contributor Author

@samba2 Unit Tests are especially hard here, as they would probably rely on mocks entirely due to TypeMirrors and stuff. If we decide to keep it, it might be sensible to create an extra module for roundtrip tests. As we're not there yet, i did not want to put the extra effort in and break the projects structure.
However, as i have written before, the best way to see this PoC in action is to build it and use it's snapshot dependency in IIP as a living example.

@uweschaefer
Copy link
Contributor Author

for the record: @otbe voted +1

uweschaefer and others added 2 commits September 25, 2020 09:53
Co-authored-by: Daniel Gronau <Daniel.Gronau@gmx.de>
Co-authored-by: Daniel Gronau <Daniel.Gronau@gmx.de>
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