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

Allow use of java.nio.file.Path as parser source, generator target (for Jackson databind#2013) #680

Merged
merged 1 commit into from
Feb 27, 2021

Conversation

sdoeringNew
Copy link

Is there no need to write Unit tests? The changed TextualTSFactory has barely coverage.

@sdoeringNew
Copy link
Author

After getting the permission to add some Unit tests those tests have been added.

Have you noticed that the Unit tests in this project are still JUnit 3?

And adding base tests for all createGenerator(..) and createParser(..) methods.
@cowtowncoder
Copy link
Member

cowtowncoder commented Feb 27, 2021

Junit dependency itself is to 4.x isn't it? As to test style, if it ain't broke... if there are specific improvement ideas those are always good to suggest of course; and if there are questions of why certain things are certain way you can add questions here or ask on gitter chat etc.

@cowtowncoder cowtowncoder merged commit 1503946 into FasterXML:master Feb 27, 2021
@cowtowncoder cowtowncoder changed the title #2013: add support for reading from / writing to a Path Add support for reading from / writing to a Path (for Jackson databind#2013) Feb 27, 2021
@cowtowncoder cowtowncoder changed the title Add support for reading from / writing to a Path (for Jackson databind#2013) Allow use of java.nio.file.Path as parser source, generator target (for Jackson databind#2013) Feb 27, 2021
@cowtowncoder cowtowncoder added this to the 3.0.0 milestone Feb 27, 2021
cowtowncoder added a commit that referenced this pull request Feb 27, 2021
@cowtowncoder
Copy link
Member

Merged; big thank you for increased test coverage! Although methods are quite trivial, there have been cases where even simplest implementations have had some odd edge cases (occasionally silly things like infinite recursion due to refactoring etc).

@cowtowncoder
Copy link
Member

Quick note: missed the fact that BinaryTSFactory also needs new methods... shouldn't be too difficult to add, but just noticed when trying to compile binary format backends.
Just noting for posterity.

@sdoeringNew
Copy link
Author

Quick note: missed the fact that BinaryTSFactory also needs new methods... shouldn't be too difficult to add, but just noticed when trying to compile binary format backends.
Just noting for posterity.

Good to know. I'll look at it the next two weeks.

@cowtowncoder
Copy link
Member

@sdoeringNew no need; I added the methods :)
(plus added "tests" that subclass binary/textual base types, so if adding abstract methods should notice missing implementations [unless IDE tries to be too helpful etc])

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.

2 participants