-
Notifications
You must be signed in to change notification settings - Fork 26
Introduction to Method Handles tutorial #90
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are some opinions on that article. I am an outside contributor and I don't have any authority over this repository.
I noticed quite a few lines ending with a single space. Is this deliberate?
When introducing methods (e.g. in the Lookup
, MethodHandle
or MethodHandles
classes), it might be a good idea to link Javadocs.
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Nataliia,
I briefly left a few comments, but would also like to add the following general ones:
- Please add links to JavaDoc for respective classes or methods.
- Could you please add a bio similar to the one from JFall? 😄
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
Hi @danthe1st and @ammbra, thank you both for the reviews. |
Hi @ammbra and @danthe1st, I have pushed the changes with your comments addressed, could you please check? Along with your suggestions I have also reordered some sections: the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your changes. I have commented on minor things I noticed while reading through your changes.
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While reading through your article again, I found a few things that could possibly be reworded.
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
Thanks @danthe1st! I have pushed the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this article and the changes you have made. For what I am concerned, this article looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Nataliia,
Thank you so much for updating the article. I left some small comments/suggestions, but overall is great work.
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
We're close!! After these last comments are resolved I think we'll look to get this merged! |
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Nataliia,
Thank you so much for your patience. I corrected some of typos and left one suggested. Would also like to request from you some changes for the conclusion (sorry for not catching this earlier).
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
Hi @ammbra, thank you for the review and your help with fixes. I have made the changes, please let me know if it looks good or more adjustment required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read through the article again and wrote down a few things I noticed.
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
app/pages/learn/01_tutorial/04_mastering-the-api/02_invoke/00_methodhandle.md
Outdated
Show resolved
Hide resolved
Hi @smthelusive if you give me access to push commits I can go in and clean a few things up and help get this merged. I think we've gone back and forth long enough and I thank you for your patience and commitment to getting this over the line! |
Thank you for your help @carimura, I have sent you an invite. I'm still willing to fix whatever is missing, so feel free to leave anything on me, I will get to it as soon as I can. |
OK I added a few grammar edits and reduced a few lines of code that felt repeated in the same section. Good from my end! I see @ammbra left a few comments that I think are still unresolved (I agree the conclusion could be a bit stronger) but other than that, we should be good to go! If you @smthelusive are busy this week (what remains of it :) ) we might be able to assist Friday and get things merged. |
…methodhandle.md Co-authored-by: dan1st <daniel@wwwmaster.at>
…methodhandle.md Co-authored-by: dan1st <daniel@wwwmaster.at>
…methodhandle.md Co-authored-by: dan1st <daniel@wwwmaster.at>
…methodhandle.md Co-authored-by: dan1st <daniel@wwwmaster.at>
…methodhandle.md Co-authored-by: dan1st <daniel@wwwmaster.at>
ba66c0d
to
14f7899
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thank you.
Method Handles article ready for review.