Skip to content

Conversation

MarkusGnigler
Copy link
Contributor

I removed the obsolete Paginate method from issue Remove obsolete Paginate builder action..

Hope this change was sufficient.

@MarkusGnigler MarkusGnigler changed the title Markusgnigler/remove paginate Remove paginate Dec 26, 2021
@fiseni
Copy link
Collaborator

fiseni commented Dec 30, 2021

Hey @MarkusGnigler,

Thank you for your help. In the issue, I also mentioned the IsPagingEnabled property. Please can you remove that too? It's no longer necessary.

@MarkusGnigler
Copy link
Contributor Author

Hy @fiseni,

oh sure, it's a better exception message. Thanks for your guidance.

I also remove the IsPagingEnabled prop, sorry for the overlooking.

Messages were changed

from:
Duplicate use of the Skip(). Ensure you don't use Skip() in the same specification!
To:
Duplicate use of Skip(). Ensure you don't use Skip() more than once in the same specification!
@MarkusGnigler
Copy link
Contributor Author

Should I also refactor the skip and take builder tests to use more the FluentAssertion library, or is using classic assertions the preferred approach since it's shorter?

Tests are located in:

  • Ardalis.Specification.UnitTests.SpecificationBuilderExtensions_Skip
  • Ardalis.Specification.UnitTests.SpecificationBuilderExtensions_Take
[Fact]
public void ThrowsDuplicateSkipException_GivenSkipUsedMoreThanOnce()
{
    Assert.Throws<DuplicateSkipException>(() => new StoreDuplicateSkipSpec());
}
[Fact]
public void ThrowsDuplicateSkipException_GivenSkipUsedMoreThanOnce()
{
    Action sutAction = () => new StoreDuplicateSkipSpec();

    sutAction.Should()
        .Throw<DuplicateSkipException>();
}

@fiseni
Copy link
Collaborator

fiseni commented Dec 31, 2021

I don't mind really. Since we're using fluent assertions in all other places, you can refactor it if you want, so we're consistent.

@MarkusGnigler
Copy link
Contributor Author

MarkusGnigler commented Dec 31, 2021

The consistent aspect was my first intention.

Should i scan other places as well?

By me it's already evening, I'll try to commit in the next few days.

Wish you a happy new year

@fiseni
Copy link
Collaborator

fiseni commented Dec 31, 2021

No worries, take your time. Yes, you can refactor other places too. Thank you for the help, it's appreciated.

Happy New Year!

@MarkusGnigler MarkusGnigler requested a review from fiseni January 1, 2022 15:26
@fiseni
Copy link
Collaborator

fiseni commented Jan 1, 2022

Hey @MarkusGnigler. You requested a review but I don't see any new commits. Do you want me to merge it as it is, or do you want to work on the tests?

@MarkusGnigler
Copy link
Contributor Author

Hi @fiseni.

Sry, i think the requested review was a mistake.

I don't know, should i create a new PR for the test refactoring or should i stick to the existing one?

@MarkusGnigler
Copy link
Contributor Author

Hi @fiseni

I've decided to commit the test assertions to this pr as well, since it's not that big impact.

I hope it's ok for you?

@fiseni
Copy link
Collaborator

fiseni commented Jan 2, 2022

Thanks for the help!

@fiseni fiseni merged commit 66c2d90 into ardalis:main Jan 2, 2022
@MarkusGnigler
Copy link
Contributor Author

Thank you as well for your friendly communication and guidance!

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