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

IH-640: Eliminate unnecessary usage of List.length to check for empty lists #5762

Conversation

last-genius
Copy link
Contributor

Instead of traversing the entire list just to check if the first element is present, do that directly.

OCaml 5.1 has List.is_empty, here a simple lst = [] is used along with match statements.

Adds a quality gate to check that we don't add trivially unnecessary cases like this anymore.

@coveralls
Copy link

Coverage Status

coverage: 44.887%. remained the same
when pulling 27fd041 on last-genius:private/asultanov/empty-length-checking
into 2008661 on xapi-project:master.

Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

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

Thanks for this, the gate is especially appreciated. Unfortunately some changes are needed

@coveralls
Copy link

Coverage Status

coverage: 44.887%. remained the same
when pulling b57ff2c on last-genius:private/asultanov/empty-length-checking
into 2008661 on xapi-project:master.

@lindig
Copy link
Contributor

lindig commented Jul 2, 2024

I like this spring cleaning!

@lindig lindig self-requested a review July 2, 2024 09:23
@last-genius
Copy link
Contributor Author

These changes passed the BST+BVT test suites, undrafting.

@last-genius last-genius marked this pull request as ready for review July 4, 2024 08:11
Copy link
Contributor

@contificate contificate left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Reading through the diff highlights a few smaller changes worth doing within the vicinity of these changes (anti-patterns seem to clump together and cleanups tend to cascade) at a later date. Perhaps we ought to have an internal page specifically tracking these kinds of incremental cleanups?

@lindig
Copy link
Contributor

lindig commented Jul 4, 2024

I would avoid creating an internal page and rather create a ticket and use labels to find them again. Or start with a single ticket to collect them and from there create sub-tickets once they are worked on.

@contificate
Copy link
Contributor

I would avoid creating an internal page and rather create a ticket and use labels to find them again. Or start with a single ticket to collect them and from there create sub-tickets once they are worked on.

Ah, yeah.. probably a better idea than throwing them into the Confluence abyss.

@last-genius last-genius force-pushed the private/asultanov/empty-length-checking branch from 8f8341c to 72784d9 Compare July 4, 2024 09:42
@xapi-project xapi-project deleted a comment from coveralls Jul 4, 2024
@xapi-project xapi-project deleted a comment from coveralls Jul 4, 2024
@xapi-project xapi-project deleted a comment from coveralls Jul 4, 2024
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

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

Please squash the commits before merging

Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
@last-genius last-genius force-pushed the private/asultanov/empty-length-checking branch from 72784d9 to 845ffdd Compare July 4, 2024 10:12
@coveralls
Copy link

Coverage Status

coverage: 44.887%. remained the same
when pulling 845ffdd on last-genius:private/asultanov/empty-length-checking
into dd9ba19 on xapi-project:master.

@last-genius last-genius merged commit af4860b into xapi-project:master Jul 8, 2024
15 checks passed
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.

5 participants