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

validating the date format, add test case, since NumberFormatExceptio… #2538

Merged
merged 3 commits into from Nov 18, 2023
Merged

Conversation

Carpe-Wang
Copy link
Contributor

validating the date format, add test case and since NumberFormatException extends IllegalArgumentException, it is only necessary to write IllegalArgumentException.

Purpose

  • Throw exception if public GsonBuilder setDateFormat(String pattern) input pattern is invalid.
  • NumberFormatException extends llegalArgumentException, it is only necessary to write llegalArgumentException in public class ISO8601Utils

Checklist

  • New code follows the Google Java Style Guide
  • If necessary, new public API validates arguments, for example rejects null
  • New public API has Javadoc
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

…n extends IllegalArgumentException, it is only necessary to write IllegalArgumentException.
@Carpe-Wang
Copy link
Contributor Author

Sorry everyone, I make a mistake when I solve conflict. So I close first pr, and make a new pr. I am so sorry for my mistake.

@Carpe-Wang Carpe-Wang closed this Nov 18, 2023
@Marcono1234
Copy link
Collaborator

Marcono1234 commented Nov 18, 2023

@Carpe-Wang why did you close this PR? I think you can just run mvn spotless:apply, commit and push the changes and it should be fine then.

Sorry though for the trouble; in the meantime while your PR was open checks had been added which verify that the code is properly formatted, which are now causing the build failure.

@Carpe-Wang Carpe-Wang reopened this Nov 18, 2023
@Carpe-Wang
Copy link
Contributor Author

@Carpe-Wang why did you close this PR? I think you can just run mvn spotless:apply, commit and push the changes and it should be fine then.

Sorry though for the trouble; in the meantime while your PR was open checks had been added which verify that the code is properly formatted, which are now causing the build failure.

Because, I thought this submission was fairly straightforward, but it seemed to have been overlooked by the collaborators, so I closed it. I have now reopened it and am reviewing and addressing the areas that may have caused issues.

Copy link
Collaborator

@Marcono1234 Marcono1234 left a comment

Choose a reason for hiding this comment

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

Thanks!

Technically the current GsonBuilder implementation also allows blank strings as 'no-pattern':

if (datePattern != null && !datePattern.trim().isEmpty()) {

But I am not sure if that is something we have to keep supporting and if anyone really relies on this (especially since it was not publicly documented).

@eamonnmcmanus, would it make sense to run the changes of this PR against the Google tests to be safe anyway?

Edit: Nevermind new SimpleDateFormat(...) actually considers empty and blanks strings as valid, so it is still possible to use them.

@eamonnmcmanus
Copy link
Member

@eamonnmcmanus, would it make sense to run the changes of this PR against the Google tests to be safe anyway?

I've already done that, and it didn't show up any problems.

@eamonnmcmanus
Copy link
Member

Thanks for this contribution!

@eamonnmcmanus eamonnmcmanus merged commit 335ff0f into google:main Nov 18, 2023
9 checks passed
@Carpe-Wang
Copy link
Contributor Author

Thanks for this contribution!

It's my honor. By the way out of curiosity, what do I need to do to become a contributor to this project? What more effort is required?

@eamonnmcmanus
Copy link
Member

By the way out of curiosity, what do I need to do to become a contributor to this project? What more effort is required?

Well, you already are a contributor, since you've just contributed this PR. Perhaps I'm not understanding the question?

@Carpe-Wang
Copy link
Contributor Author

By the way out of curiosity, what do I need to do to become a contributor to this project? What more effort is required?

Well, you already are a contributor, since you've just contributed this PR. Perhaps I'm not understanding the question?

I mean display in here
image

@eamonnmcmanus
Copy link
Member

I think the people in that box are chosen automatically by GitHub based on who has contributed the most. If you open up where it says "+ 132 contributors" you should see everyone else. I don't see you there yet, but probably it only gets updated every so often.

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.

None yet

3 participants