Skip to content

Remove paginate - #195

Merged
fiseni merged 6 commits into
ardalis:mainfrom
MarkusGnigler:markusgnigler/remove-paginate
Jan 2, 2022
Merged

Remove paginate#195
fiseni merged 6 commits into
ardalis:mainfrom
MarkusGnigler:markusgnigler/remove-paginate

Conversation

@MarkusGnigler

Copy link
Copy Markdown
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

fiseni commented Dec 30, 2021

Copy link
Copy Markdown
Collaborator

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.

Comment thread Specification/src/Ardalis.Specification/Exceptions/DuplicateSkipException.cs Outdated
Comment thread Specification/src/Ardalis.Specification/Exceptions/DuplicateTakeException.cs Outdated
@MarkusGnigler

Copy link
Copy Markdown
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
Copy Markdown
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

fiseni commented Dec 31, 2021

Copy link
Copy Markdown
Collaborator

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

MarkusGnigler commented Dec 31, 2021

Copy link
Copy Markdown
Contributor Author

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

fiseni commented Dec 31, 2021

Copy link
Copy Markdown
Collaborator

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

fiseni commented Jan 1, 2022

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown
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

fiseni commented Jan 2, 2022

Copy link
Copy Markdown
Collaborator

Thanks for the help!

@fiseni
fiseni merged commit 66c2d90 into ardalis:main Jan 2, 2022
@MarkusGnigler

Copy link
Copy Markdown
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