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

[#112]Corrected skip methods behavior of JsonParserImpl; #113

Merged
merged 5 commits into from
Feb 26, 2024

Conversation

api-from-the-ion
Copy link
Contributor

Corrected the behavior of the skip methods in the parser implementation

@api-from-the-ion
Copy link
Contributor Author

I'm writing the tests here, and I'm missing Junit 5 dearly. My question: could we move to Junit 5? This should be just a simple work of changing dependencies and modifying the 380 tests to use annotations and so on.

/**
* Class with methods that creates JsonParser with different configuration from different sources and runs the test code with this parser
*/
public class JsonParserFixture {

Choose a reason for hiding this comment

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

That is a self-explanatory name :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I know. ;-) But what else should I write here? This is a fixture class for the tests involving the parser, so many of them. I don't like to repeat this try-with-resource line over and over again.

Or did you mean like we don't need the JavaDoc for the class itself, only the methods of this particular class? I mean, it's maybe not necessary, but this comment also doesn't hurt anybody, right? ;-)

Copy link
Member

@lukasj lukasj left a comment

Choose a reason for hiding this comment

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

  • avoid whitespace changes - use spaces and not tabs
  • avoid static imports - they just make reviewers life more difficult when mixed with other changes
  • are pom updates required here? they are usually handled through separate PRs (or through PRs which are to be just rebased instead of being squashed)

@lukasj
Copy link
Member

lukasj commented Nov 14, 2023

My question: could we move to Junit 5? This should be just a simple work of changing dependencies and modifying the 380 tests to use annotations and so on.

If someone provides PR for this, I see no reason to not to move there.

@api-from-the-ion
Copy link
Contributor Author

api-from-the-ion commented Nov 14, 2023

  • avoid whitespace changes - use spaces and not tabs

    • avoid static imports - they just make reviewers life more difficult when mixed with other changes

    • are pom updates required here? they are usually handled through separate PRs (or through PRs which are to be just rebased instead of being squashed)

  • I'll convert them. Yasson using the Checkstyle rule for this - shouldn't it be activated here also?
  • I remove the static imports and make a commit
  • Should I create a new branch with POM changes and cherry-pick these changes over there? I forgot to create a branch for my changes and done it on the changes on the master directly. ;-(

P.S.: Created a branch with Checkstyle. It has immediately 468 errors. Could anybody take a look on the rules, if they are OK? If so, I would correct the errors and make a PR of corrections and Checkstyle.

@api-from-the-ion
Copy link
Contributor Author

api-from-the-ion commented Nov 14, 2023

If someone provides PR for this, I see no reason to not to move there.

OK, so we don't need some ticket for this? Just the PR full of commits to review? And another question: should I create a branch from the current master or is it OK, to fork some of the branches?

I see, I explained myself not so clearly. I have two (independent) changes in my mind. First: to update a JUnit. And independently, move from "inherit from TestCase" schema to the "use annotations" schema on tests. Anybody has some objections on the second case?

@lukasj lukasj merged commit dfbe33a into eclipse-ee4j:master Feb 26, 2024
1 check passed
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.

3 participants