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

Add wildcard parameterized type #121

Merged
merged 1 commit into from
Feb 20, 2022
Merged

Add wildcard parameterized type #121

merged 1 commit into from
Feb 20, 2022

Conversation

yhtMinceraft1010X
Copy link
Contributor

@yhtMinceraft1010X yhtMinceraft1010X commented Feb 3, 2022

Part of #122

Proposed commit message:

The assertParseSuccess and assertParseFailure parameter parser
uses raw type for Parser. This would give a warning under to 
javac -Xlint:rawtypes.

Let's add a wildcard type parameter to Parser.

Screenshot (68)

@canihasreview
Copy link

canihasreview bot commented Feb 3, 2022

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

@canihasreview
Copy link

canihasreview bot commented Feb 3, 2022

v1

@yhtMinceraft1010X submitted v1 for review.

(📚 Archive)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/121/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

@damithc
Copy link
Contributor

damithc commented Feb 3, 2022

Thanks very much for the PR @yhtMinceraft1010X
@se-edu/tech-team-level1 for your review please ...

@jayasting98
Copy link

That seems to remove that warning. This change seems fine to me.

Copy link
Contributor

@LimJunxue LimJunxue left a comment

Choose a reason for hiding this comment

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

LGTM ~

Copy link
Contributor

@JinHao-L JinHao-L left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@damithc
Copy link
Contributor

damithc commented Feb 15, 2022

Thanks all for the reviews. @yhtMinceraft1010X perhaps we can omit Intellij from the commit message? Rationale: as this a improvement to the code, we should be able to justify it without relying on an IDE warning.
Don't forget to wrap the body at 70 chars, not 50 chars.

@yhtMinceraft1010X
Copy link
Contributor Author

Thanks Prof! I have fixed the commit message

IntelliJ throws a warning that assertParseSuccess
and assertParseFailure parameter parser uses
raw type for Parser. This warning is similar to
javac -Xlint:rawtypes.

Let's add a wildcard type parameter to Parser.
@canihasreview
Copy link

canihasreview bot commented Feb 16, 2022

v2

@yhtMinceraft1010X submitted v2 for review.

(📚 Archive) (📈 Interdiff between v1 and v2) (📈 Range-Diff between v1 and v2)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level3.git refs/pr/121/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

@damithc damithc merged commit 865de62 into se-edu:master Feb 20, 2022
@damithc
Copy link
Contributor

damithc commented Feb 20, 2022

Thanks for the fix @yhtMinceraft1010X
Note that I revised the commit message a bit when merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants