Skip to content

Order by multiple properties sequentially #1416

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.

Sign up for GitHub

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

Jump to bottom
Merged
dennisdoomen merged 4 commits into from
Oct 28, 2020
Merged

Order by multiple properties sequentially #1416

dennisdoomen merged 4 commits into from
Oct 28, 2020

Conversation

@Serg046
Copy link
Contributor

@Serg046 Serg046 commented Oct 27, 2020 •
edited
Loading

IMPORTANT

  • The code complies with the Coding Guidelines for C#.
  • The changes are covered by a new or existing set of unit tests which follow the Arrange-Act-Assert syntax such as is used in this example.
  • If the contribution adds a feature or fixes a bug, please update the release notes, which are published on the website.
  • If the contribution changes the public API the changes needs to be included by running AcceptApiChanges.ps1/AcceptApiChanges.sh.
  • If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.

Issue #1160

@Serg046
Copy link
Contributor Author

Serg046 commented Oct 27, 2020 •
edited
Loading

ThenNotBeInAscendingOrder and ThenNotBeInDescendingOrder are not provided because it will significantly complicate the change. The issue is the following lines of NotBeOrderedBy:

Execute.Assertion
    .ForCondition(!unordered.SequenceEqual(expectation))
    .BecauseOf(because, becauseArgs)
    .FailWith("Expected {context:collection} {0} to not be ordered {1}{reason} and not result in {2}.",
        Subject, orderString, expectation);

It would be much more complex than just !unordered.SequenceEqual(expectation).
Anyway, I don't think it is worth implementing because even if they were implemented, it would be difficult to understand their behavior therefore they would be in rare use.

dennisdoomen
dennisdoomen requested changes Oct 28, 2020
View reviewed changes
Copy link
Member

@dennisdoomen dennisdoomen left a comment

Choose a reason for hiding this comment

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

👍 Nice work.
❌ Please update the title to be functional

Src/FluentAssertions/Collections/SubsequentOrderingGenericCollectionAssertions.cs Outdated
namespace FluentAssertions.Collections
{
[DebuggerNonUserCode]
public class SubsequentOrderingGenericCollectionAssertions<T> :
Copy link
Member

@dennisdoomen dennisdoomen Oct 28, 2020

Choose a reason for hiding this comment

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

🔧 Maybe rename it to SubsequentOrderingAssertions?

docs/_pages/releases.md Outdated
* Added `ComparingBy{Members,Value}(Type)` to allow specifying open generic types - [#1389](https://github.com/fluentassertions/fluentassertions/pull/1389).
* Added overload of `CollectionAssertions.NotBeEquivalentTo` that takes a `config` parameter` - [#1408](https://github.com/fluentassertions/fluentassertions/pull/1408).
* Changed `StringAssertions.StartWith`, `StringAssertions.EndWith` and their `EquivalentOf` versions to allow empty strings - [#1413](https://github.com/fluentassertions/fluentassertions/pull/1413).
* Added ThenBeInAscendingOrder and ThenBeInDescendingOrder to allow asserting that a subsequence is ordered in ascending or descending order - [#1416](https://github.com/fluentassertions/fluentassertions/pull/1416).
Copy link
Member

@dennisdoomen dennisdoomen Oct 28, 2020

Choose a reason for hiding this comment

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

🙃 Would be nice to format those names using back-ticks.

Tests/FluentAssertions.Specs/Collections/GenericCollectionAssertionsSpecs.cs Outdated
#region Then be in order

[Fact]
public void When_asserting_subsequence_by_then_be_in_ascending_order_it_should_pass()
Copy link
Member

@dennisdoomen dennisdoomen Oct 28, 2020

Choose a reason for hiding this comment

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

🤔 Try to keep the names of test functional and never refer to the literal name of a method or API, e.g.

When_the_collection_is_ordered_according_to_the_subsequent_ordering_assertion_it_should_succeed

jnyrup
jnyrup reviewed Oct 28, 2020
View reviewed changes
Copy link
Member

@jnyrup jnyrup left a comment

Choose a reason for hiding this comment

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

Does this support multiple ThenBeInAscendingOrder?
E.g.

collection.Should()
	.BeInAscendingOrder(x => x.Item1)
	.And
	.ThenBeInAscendingOrder(x => x.Item2)
	.And
	.ThenBeInAscendingOrder(x => x.Item3);

@Serg046
Copy link
Contributor Author

Serg046 commented Oct 28, 2020

Applied the notes and added two tests showing multiple ThenBeInAscending(Descending)Order support

@Serg046 Serg046 changed the title Issue 1160 Order by multiple properties sequentially Oct 28, 2020
@dennisdoomen dennisdoomen merged commit 0a1fa2c into fluentassertions:develop Oct 28, 2020
@jnyrup jnyrup mentioned this pull request Nov 11, 2023
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dennisdoomen dennisdoomen dennisdoomen approved these changes

@jnyrup jnyrup jnyrup approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Serg046 @dennisdoomen @jnyrup