-
Notifications
You must be signed in to change notification settings - Fork 282
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
ELY-2629: Move method permissionFor in IntNameSetPermissionCollection class #1963
ELY-2629: Move method permissionFor in IntNameSetPermissionCollection class #1963
Conversation
hi @deepalik-neu, thank you for your contribution! can you please link the JIRA issue in the PR description? |
@deepalik-neu thank you so much Deepali! Just a minor, the commit message should include the jira issue number. Let us know if you need any help with that |
Thank you for letting me know. |
@deepalik-neu You can rename your commit by using "git commit --amend" and add an issue number there. Thank you! |
@deepalik-neu good job! we just now need to squash the commits together. You can follow the steps from Sonia on slack :) You can start with the "git rebase -i HEAD~2" |
@deepalik-neu I just noticed that 1 of your current commits is a "merge" commit. So unfortunately the usual interactive rebase with squash will not work. So please follow these steps to squash your commits: git checkout 2.x For explanation, see this stackoverflow response |
d11d3ad
to
d82fc6f
Compare
Done. Thank you! |
@@ -94,6 +90,10 @@ public boolean hasMoreElements() { | |||
return bits != 0; | |||
} | |||
|
|||
private Permission permissionFor(int id) { |
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.
@deepalik-neu Just a total minor, since this method is private
, can you please move it under all the public methods? It is our code style to have private methods located under the public methods. Thank you!
@deepalik-neu After you make the changes, you can use the |
d82fc6f
to
c735f04
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 good to me! Thanks @deepalik-neu
@deepalik-neu There are some intermittent failures in the CI, those are unrelated to your changes. So I am re-running the jobs until they pass and then I can merge :) |
Thank you! @Skyllarr |
https://issues.redhat.com/browse/ELY-2629