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.

Sign up for GitHub

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

Jump to bottom

[#1004] clean up compiler warnings #1030

Closed
wants to merge 8 commits into from
Closed

[#1004] clean up compiler warnings #1030

wants to merge 8 commits into from

Conversation

seanpmcn
Copy link

@seanpmcn seanpmcn commented Aug 7, 2023 •
edited by lprimak
Loading

fixes #1004

Following this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a GitHub issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a GitHub issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [#XXX] - Fixes bug in SessionManager,
    where you replace #XXX with the appropriate GitHub issue. Best practice
    is to use the GitHub issue title in the pull request title and in the first line of the commit message.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • add fixes #XXX if merging the PR should close a related issue.
  • Run mvn verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If you have a group of commits related to the same change, please squash your commits into one and force push your branch using git rebase -i.
  • Committers: Make sure a milestone is set on the PR

Trivial changes like typos do not require a GitHub issue (javadoc, comments...).
In this case, just format the pull request title like [DOC] - Add javadoc in SessionManager.

If this is your first contribution, you have to read the Contribution Guidelines

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@seanpmcn
Copy link
Author

seanpmcn commented Aug 7, 2023

I am still new to GitHub PRs. Most of my experience is with BitBucket. How do I assign a milestone, and how do I know which milestone to use?

@seanpmcn seanpmcn changed the title 1004 clean up compiler warnings [1004] clean up compiler warnings Aug 7, 2023
@seanpmcn seanpmcn changed the title [1004] clean up compiler warnings [#1004] clean up compiler warnings Aug 7, 2023
@lprimak
Copy link
Contributor

lprimak commented Aug 7, 2023

That’s for committers. Don’t worry about that

@seanpmcn
Copy link
Author

seanpmcn commented Aug 7, 2023

@lprimak
Would I not be considered a committer?

@lprimak
Copy link
Contributor

lprimak commented Aug 7, 2023

Not yet :) You will be a Contributor.

lprimak
lprimak reviewed Aug 7, 2023
View reviewed changes
config/ogdl/src/main/java/org/apache/shiro/config/ogdl/CommonsInterpolator.java Outdated
import org.apache.commons.configuration2.interpol.ConstantLookup;
import org.apache.commons.configuration2.interpol.EnvironmentLookup;
import org.apache.commons.configuration2.interpol.SystemPropertiesLookup;
import org.apache.commons.configuration2.interpol.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use star imports. Those are usually considered bad form. Individual imports are great.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for pointing that out! I didn't notice my IDE adding that.

@lprimak lprimak self-assigned this Aug 7, 2023
@lprimak lprimak added pending-cla shiro-2.0.0 java Pull requests that update Java code core Core Modules labels Aug 7, 2023
@lprimak lprimak added this to the 2.0 milestone Aug 7, 2023
@lprimak
Copy link
Contributor

lprimak commented Aug 7, 2023

Thank you for your contribution!

@seanpmcn
Copy link
Author

seanpmcn commented Aug 7, 2023

@lprimak Alright. So everything else applies?

@seanpmcn
Copy link
Author

seanpmcn commented Aug 7, 2023

@lprimak If you don't mind me asking, what is the procedure for responding to PR comments. Should I just make updates, commit, and push?

dependabot bot and others added 3 commits August 7, 2023 18:28
@dependabot @seanpmcn
build(deps): bump org.yaml:snakeyaml from 2.0 to 2.1
e42ff7e 
Bumps [org.yaml:snakeyaml](https://bitbucket.org/snakeyaml/snakeyaml) from 2.0 to 2.1.
- [Commits](https://bitbucket.org/snakeyaml/snakeyaml/branches/compare/snakeyaml-2.1..snakeyaml-2.0)

---
updated-dependencies:
- dependency-name: org.yaml:snakeyaml
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
@seanpmcn
[apache#1004] Address AbstractHash issues by updating to SimpleHash
e3bf49c
@seanpmcn
[apache#1004] Remove wildcard
b6e5322
@lprimak
Copy link
Contributor

lprimak commented Aug 7, 2023

If you don't mind me asking, what is the procedure for responding to PR comments. Should I just make updates, commit, and push?

yes :)

Alright. So everything else applies?

Not sure what you mean there. The issue is to fix compiler warnings, so if you fix the compiler warning that applies then :)

Try not to do too much. I did see some class renaming. I am pretty sure that would be out of scope.

@seanpmcn
Copy link
Author

seanpmcn commented Aug 7, 2023

Try not to do too much. I did see some class renaming. I am pretty sure that would be out of scope.

Oh, I'll have to take a look at that.

By the way, would it be expected I resolve the issue or just make progress? I know some places prefer frequent pull requests while others prefer them to be basically when the entire issue is resolved.

@lprimak
Copy link
Contributor

lprimak commented Aug 7, 2023

I would say to air on the side of resolving the issue in full

@seanpmcn
Copy link
Author

seanpmcn commented Aug 7, 2023

Alright. I might retract this PR then. I'm not sure I want to hanging around while I work on that

@lprimak
Copy link
Contributor

lprimak commented Aug 7, 2023

I would just keep working on it. No harm there in draft

@seanpmcn
Copy link
Author

seanpmcn commented Aug 8, 2023

Good point. Would i have to keep squashing or could I just keep my future commits separate?

@lprimak
Copy link
Contributor

lprimak commented Aug 8, 2023

I would not worry about squashing until you are ready at the end

@seanpmcn
Copy link
Author

seanpmcn commented Aug 8, 2023

Alright. Thanks for taking the time to reply to me! I find it really encouraging.

@lprimak
Copy link
Contributor

lprimak commented Aug 8, 2023

No problem. Looking forward to your contributions!

@seanpmcn
Copy link
Author

seanpmcn commented Aug 10, 2023 •
edited
Loading

Try not to do too much. I did see some class renaming. I am pretty sure that would be out of scope.

Could you point out where that is? I don't believe I've renamed any classes, but I'm sure I may be wrong.

@seanpmcn
Copy link
Author

Also, it seems that subclasses of HashedCredentialMatcher have been deprecated. I believe removing the implementation of those classes is out of the scope of this issue.

@lprimak
Copy link
Contributor

lprimak commented Aug 11, 2023

Just leave the questionable stuff for last

@seanpmcn
Copy link
Author

It seems like most errors are outdated documentation that includes deprecated items. I don't think I have sufficient knowledge of the code base to update the documentation. Maybe I could publish the PR without the documentation changes. Let me know what would be the best course of action.

@lprimak
Copy link
Contributor

lprimak commented Aug 12, 2023

I would concentrate on resolving unchecked casts and raw types, by making them actual generics or using @SuppressWarnings where appropriate

@seanpmcn
Copy link
Author

Thank you. That's clears up what direction I should take for this.

@lprimak lprimak added the help-wanted Help Wanted label Oct 4, 2023
@lprimak
Copy link
Contributor

lprimak commented Oct 4, 2023

This effort is abandoned for now, Anyone willing please feel free to take it over.

@lprimak
Copy link
Contributor

lprimak commented Dec 16, 2023

Superseded by #1233

@lprimak lprimak closed this Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lprimak lprimak lprimak left review comments

Assignees

@lprimak lprimak

Labels
abandoned core Core Modules help-wanted Help Wanted java Pull requests that update Java code pending-cla shiro-2.0.0
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.

Clean up compiler warnings
2 participants
@seanpmcn @lprimak