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

Improvement/add-methods-consuming-collection #290

Merged
merged 9 commits into from
Oct 11, 2019

Conversation

MicWalter
Copy link
Contributor

@MicWalter MicWalter commented Jun 11, 2019

Motivation
It should be easy to add a dynamic amount of topics to a Subscribe/Unsubscribe packet or User Properties to any packet.

Resolves #170

Changes
Added methods that consume collection for:

  • Mqtt3/Mqtt5 Subscribe Builder
  • Mqtt3/Mqtt5 Unsubscribe Builder
  • UserProperties Builder

@MicWalter MicWalter requested a review from SgtSilvio June 11, 2019 20:01
@MicWalter MicWalter requested a review from dobermai as a code owner June 11, 2019 20:01
@MicWalter MicWalter self-assigned this Jun 11, 2019
@cla-bot cla-bot bot added the cla-signed label Jun 11, 2019
@MicWalter
Copy link
Contributor Author

Related to #170

@MicWalter MicWalter force-pushed the improvement/add-methods-consuming-collection branch from 5b7d49c to 74bfb24 Compare June 13, 2019 10:39
@SgtSilvio
Copy link
Member

Nice PR 👍
Some general feedback:

  • We should take Collection as argument instead of List so passing a Set is also possible
  • We should additionally provide methods with an array (varargs) as an argument
  • We should additionally provide methods with a Stream as an argument. This allows the user to e.g. specify a list of topic filters with the same QoS:
List<String> topicFilters = Arrays.asList("test", "hello", "world");
MqttQos qos = MqttQos.EXACTLY_ONCE;
Mqtt5Subscribe.builder()
        .addSubscriptions(topicFilters.stream().map(topicFilter -> 
                Mqtt5Subscription.builder().topicFilter(topicFilter).qos(qos).build()))
        .build();

This would also allow us to only take Collection<MqttTopicFilter> and not Collection<String> as argument to addTopicFilters:

List<String> topicFilters = Arrays.asList("test", "hello", "world");
Mqtt5Unsubscribe.builder().addTopicFilters(topicFilters.stream().map(MqttTopicFilter::of)).build();

Copy link
Member

@SgtSilvio SgtSilvio left a comment

Choose a reason for hiding this comment

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

See my comment above

@SgtSilvio SgtSilvio removed the request for review from dobermai June 19, 2019 08:10
@SgtSilvio SgtSilvio added this to the 1.2 milestone Jun 24, 2019
@SgtSilvio SgtSilvio force-pushed the improvement/add-methods-consuming-collection branch from 39354a7 to 54aeb1d Compare August 9, 2019 21:54
@SgtSilvio SgtSilvio removed this from the 1.2 milestone Aug 12, 2019
@SgtSilvio
Copy link
Member

I removed the label and milestone as we have a ticket for tracking this data: #170
PRs are only labelled and included in projects and milestones if no corresponding issue exists, so the PR acts as both ticket and PR.

@SgtSilvio SgtSilvio force-pushed the improvement/add-methods-consuming-collection branch from 5fa8024 to 819edf4 Compare September 26, 2019 19:39
@SgtSilvio SgtSilvio force-pushed the improvement/add-methods-consuming-collection branch from 819edf4 to 845f1f9 Compare October 11, 2019 20:40
@SgtSilvio SgtSilvio changed the base branch from develop to develop-1-2 October 11, 2019 20:48
@SgtSilvio SgtSilvio merged commit f76385f into develop-1-2 Oct 11, 2019
@SgtSilvio SgtSilvio deleted the improvement/add-methods-consuming-collection branch October 11, 2019 21:27
@SgtSilvio SgtSilvio mentioned this pull request Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants