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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Created DefaultValueGenerator<T> class to handle construction of problematic types #521

Merged
merged 1 commit into from Feb 2, 2016

Conversation

Pvlerick
Copy link
Contributor

In #490, I mentioned that System.Net.IPAddress was also a problematic type that AutoFixture wasn't able to create.

I started out by creating a new type to handle IPAddress specifically just as CulstureInfo and Encoding (#512) were handled, but it very quickly looked like code duplication (rule of three, I guess).

So I went ahead and created a new generic type that has a constructor taking a default value and the Create method will always return that default value. I deleted the previous generators and moved relevant tests in the new class' tests.

Open questions:

  • Does the DefaultValueGenerator name sound good? I started out with CustomGenerator but changed my mind half-way, naming is hard 馃槃
  • Should the constructor take a lambda factory instead of a plain value, so that it can return a new instance every time if needed?

CC @tiesmaster

@adamchester
Copy link
Member

Looks great @Pvlerick! It looks this is a breaking change, since the deleted classes are public. Would it make sense to keep those classes (either as-is, or use the new class as an implementation?).

@Pvlerick
Copy link
Contributor Author

@adamchester you are totally right, it was heavy handed to delete these two 馃憥

What about marking them as obsolete?

@adamchester
Copy link
Member

Obsolete sounds good.

@Pvlerick
Copy link
Contributor Author

I marked both InvariantCultureGenerator and Utf8EncodingGenerator as Obsolete and removed their corresponding unit tests (based on what I could see in the history, this seems to be standard way to proceed to avoid the warnings in the test project).

@ploeh
Copy link
Member

ploeh commented Jan 31, 2016

Do we need a new class for this? Isn't it enough to compose an ISpecimenBuilder like the following?

new FilteringSpecimenBuilder(
    new FixedBuilder(value),
    new ExactTypeSpecification(type)));

@Pvlerick
Copy link
Contributor Author

Looks like it 馃憤

I will simplify tomorrow and update the PR.

@tiesmaster
Copy link
Contributor

@Pvlerick: 馃憤

@adamchester
Copy link
Member

Regarding Obsolete, I thought in the past we actually added #warning disable in the tests to suppress the warnings?

@ploeh
Copy link
Member

ploeh commented Jan 31, 2016

in the past we actually added #warning disable in the tests to suppress the warnings

We still do. The code is still there, so should still be covered by tests, but we don't want the compiler warnings from our tests.

@Pvlerick
Copy link
Contributor Author

I looked at another Obsolete marked type, DateTimeGenerator and in this case the tests were deleted. However that might be a bad example since it has the error flag in the Obsolete attribute.

In any event, I'll update the PR and restore the tests. Thank you for the guidance 馃憤

@ploeh
Copy link
Member

ploeh commented Feb 1, 2016

The progression is this:

  1. [Obsoloete, false], using #pragma warn disable 618. This can be done as soon as we detect the need to deprecate a type or member. It's not a breaking change, but rather advance warning to users that something's going to change. The sooner such a warning is published, the better.
  2. [Obsolete, true]. At this point, code that uses the deprecated API can no longer compile, so there's no option but to delete the tests. This is a breaking change, so can only be done when we increment the major version of AutoFixture.
  3. Delete the type or member. This is also a breaking change (I think), so should only be done when we increment the major version.

I think that DateTimeGenerator was deprecated already in AutoFixture 2; in AutoFixture 4 we can entirely delete it.

HTH

@Pvlerick Pvlerick force-pushed the default-value-generator branch 2 times, most recently from 0860ef1 to 8edbae2 Compare February 1, 2016 07:46
@Pvlerick
Copy link
Contributor Author

Pvlerick commented Feb 1, 2016

Makes sense, thanks for clarifying.

I updated the pull request accordingly.

Assert.IsAssignableFrom<ISpecimenBuilder>(sut);
}

[Fact]
public void CreateWithNullRequestWillReturnNoSpecimen()
{
#pragma warning disable 618
var sut = new InvariantCultureGenerator();
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't there be a #pragma warning restore 618 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I missed it, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ploeh Fixed.

@adamchester
Copy link
Member

馃憤

@ploeh ploeh merged commit 227e0ba into AutoFixture:master Feb 2, 2016
@ploeh
Copy link
Member

ploeh commented Feb 2, 2016

Thank you for your contribution! It's now live as AutoFixture 3.40.0.

@Pvlerick Pvlerick deleted the default-value-generator branch February 26, 2016 06:56
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

4 participants