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

Add iterables package containing helper method for getting the only element in a collection #520

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

Mattheo28
Copy link

Before this PR

We would have to call Iterators.getOnlyElement(collection.iterator()); directly whenever we wanted this functionality, having to also check Preconditions.checkState(collection.size() == 1); before to make sure we would get a more sensible error message. I found myself having to use this a lot.

After this PR

Now one can simply call com.palantir.logsafe.Iterables.getOnlyElement(...) with their collection and can expect this to check that there is indeed 1 element in the collection before retrieving it, otherwise we fail with a nice error.

@changelog-app
Copy link

changelog-app bot commented Feb 25, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Add iterables package containing helper method for getting the only element in a collection

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from tpetracca February 25, 2021 14:24
@Mattheo28
Copy link
Author

Tested this locally and was able to pull the new package in to a different repo and use getOnlyElement

Copy link
Contributor

@carterkozak carterkozak left a comment

Choose a reason for hiding this comment

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

I'd prefer not to duplicate guava functionality for safe logging, especially methods which don't take diagnostic message information. What if we provided a javaagent or similar to substitute our own implementation of the iterables/collections/etc methods which throw safe exceptions on failure, however don't require every developer to be aware of another utility class?

I worry that we're building too large a library of utility functionality that it becomes distracting and difficult to develop new components.

README.md Outdated
import com.google.common.collect.Iterators;
...
Preconditions.checkState(collection.size() == 1, "Expected exactly one element in collection", args);
return Iterators.getOnlyElement(collection.iterator());
Copy link
Contributor

Choose a reason for hiding this comment

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

Not Iterables?


private Iterables() {}

public static <T, C extends Collection<T>> T getOnlyElement(C collection, Arg<?>... args) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a collection, not an iterable. Guava Iterables takes an iterable, and iterators takes an iterator.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah looks like i mixed the two together -- I'd be happy to implement this for both Iterables and Iterators

@Mattheo28
Copy link
Author

That sounds like a good idea, although I haven't written a java agent before, any chance you can point me to one and I can give it a shot? I can implement this to substitute both Iterables and Iterators getOnlyElement as well!

@carterkozak
Copy link
Contributor

I can take a look at implementing this, although I can't promise a delivery date.

@Mattheo28
Copy link
Author

up to you -- i'd be happy to take a stab at it but not having any luck in finding an example of us doing this for something similar

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