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

Improve converters #613

Conversation

Projects
None yet
3 participants
@antoine-aumjaud
Copy link
Contributor

commented Feb 6, 2015

  • fix NPE when value is null for all converters or when string is empty
  • create a generic array converter (works for all arrays), use underlying converter for elements
  • create a generic collections converter (works for all collections), use underlying converter for elements
Improve converters:
- fix NPE when value is null for all converters or when string is empty
- create a generic array converter (works for all arrays), use underlying converter for elements
- create a generic collections converter (works for all collections), use underlying converter for elements
@antoine-aumjaud

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2015

I've not found the changeLog.txt

CONTRIBUTING.md: Whenever you make a change, add a few sentences to the changeLog.txt file describing your changes. We used this file to build the release notes.

@amolenaar

This comment has been minimized.

Copy link
Collaborator

commented Feb 6, 2015

CONTRIBUTING.md is not up to date. I always update the page FitNesse.ReleaseNotes. That page is also included in the distribution, which is a lot nicer.

@amolenaar amolenaar added this to the Next release milestone Feb 8, 2015

@amolenaar amolenaar merged commit dcd0883 into unclebob:master Feb 8, 2015

amolenaar added a commit that referenced this pull request Feb 8, 2015

}

public String fromString(String arg) {
return arg;
return !StringUtils.isBlank(arg) ? arg : null;

This comment has been minimized.

Copy link
@amolenaar

amolenaar Mar 21, 2015

Collaborator

I think this is the offending line for #686.

@antoine-aumjaud

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2015

Yes, that's correct. I have changed the behavior of String converter to have the same logic for all converters: empty string set object reference to null. For the issue #686, @davidmagill could override this StringConverter in ConverterRegistry. The real issue is that we can't set an empty string to a String field.

@antoine-aumjaud

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2015

I can do a pull request to add this feature.

@amolenaar

This comment has been minimized.

Copy link
Collaborator

commented Mar 22, 2015

We can't make a distinction between null and an empty string, indeed.
I agree that the behaviour is at least consistent now with the other conversion types. I proposed PR #687, but don't know if it's the right thing to merge it.

@antoine-aumjaud

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2015

Personally, I prefer to keep the consistency instead of the backward compatibility.
As I've said, If a user wants an empty string, he can override the StringConverter in the registry.
We can use a feature flag too, as suggest by @mgaertne.
But I let you the choice :), both are defensible.

@woodybrood

This comment has been minimized.

Copy link
Collaborator

commented Mar 24, 2015

I think having a flag is a minimum requirement for this. I know that the
framework I implemented at my previous employer and the hundreds of tests
written in it use this behavior and it would be a significant effort for
them to fix.

Overriding the behavior by adding your own StringConverter is something
that should be genuinely unique behavior and not an expectation when we are
changing existing functionality.

We also need to advertise this heavily in the release notes. This will
affect a lot of users and that is as important as consistency.

On Tue, Mar 24, 2015 at 4:34 AM, Antoine Aumjaud notifications@github.com
wrote:

Personally, I prefer to keep the consistency instead of the backward
compatibility.
As I've said, If a user wants an empty string, he can override the
StringConverter in the registry.
We can use a feature flag too, as suggest by @mgaertne
https://github.com/mgaertne.
But I let you the choice :), both are defensible.


Reply to this email directly or view it on GitHub
#613 (comment).

@antoine-aumjaud

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2015

Yes @woodybrood, we would have added it in the release note. My bad, I have not indicated this in the PR comment.

@amolenaar

This comment has been minimized.

Copy link
Collaborator

commented Mar 30, 2015

I did not notice it as well, among all the improvements.

I think a switch is somewhat hard, since we also have to pass it on to the SUT. Shall I apply #687 for now?

@antoine-aumjaud

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2015

I think you can apply your PR. It 's better for backward compatibility.
Thanks Arjan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.