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

Set opts if calling subscribeMany with 1 topic #121

Merged

Conversation

qm3ster
Copy link
Contributor

@qm3ster qm3ster commented Apr 22, 2021

fixes #120

Concerns:

  1. should this check live in the underlying ResponseOptionsBuilder::subscribe_many_options instead?
  2. are there any tests that need to be added for this? The underlying issue is in the C library
  3. not sure about formating, rustfmt makes a ton of changes to this and other files both on +stable and +nightly, this should probably be addressed separately. Probably not checked in CI. I tried to format sensibly by hand.

@fpagliughi
Copy link
Contributor

fpagliughi commented Apr 22, 2021

Thanks so much for the PR. But to answer your questions...

  1. Yes. This whould definitely live in ResponseOptionsBuilder otherwise that is still broken.
  2. Yes, we should add the corner case tests for len = 0 or 1 to test_sub_many_opts()
  3. Apologies about the formatting. When I started the project years back, rustfmt wasn't so good and mangled some of the code. I know it works now, but still keep putting off a complete reformat until the "next" release and/or when a few of the options are modifyable in stable.

I'm thinking the Builder should handle all cases properly. Maybe like:

    /// Sets a single set of subscribe options - for a call to subscribe_many()
    pub fn subscribe_many_options(&mut self, opts: &[SubscribeOptions]) -> &mut Self {
        match opts.len() {
            0 => (),
            1 => self.copts.subscribeOptions = opts.copts;
            _ => self.data.sub_opts = Some(opts.iter().map(|opt| opt.copts).collect());
        }
        self
    }

Does that seem right?
Let me know if you want to do this, otherwise I can get to it later today.

@qm3ster
Copy link
Contributor Author

qm3ster commented Apr 22, 2021

  1. Should subscribe_options still exist?

  2. If changing stuff, maybe something like

fn subscribe_many_options<S: Into<ffi::MQTTSubscribe_options>, I: IntoIterator<Item = S>> (&mut self, opts: I) -> &mut Self {
    let mut opts = opts.into_iter();
    match opts.size_hint() {
        (_, Some(0)) => {}
        (1, Some(1)) => self.copts.subscribeOptions = opts.next().unwrap().into().copts,
        _ => {
            let opts: Vec<_> = opts.map(Into::into).collect();
            match &opts {
                [] => {}
                [opts] => self.copts.subscribeOptions = opts,
                _ => self.data.sub_opts = Some(opts),
            }
        }
    }
}

(lol)

Or, on the other hand, it can take a Vec<ffi::MQTTSubscribe_options>, that the user may copy or fill via memcopy.
Taking a slice to then clone + transmute it via map seems like the least flexible option.

@fpagliughi
Copy link
Contributor

Oh, yeah, IntoIterator might be a good idea, but jeez the implementation is ugly in comparison (but probably worth it for the flexibility). Can you think of a use case where you would pass it something that wasn't a Vec<> or slice?

But... the whole point of the top crate is to hide the underlying C implementation. So I don't want to expose the C (ffi) types to the apps. I've been slowly removing the places where they leaked through in earlier versions. The goal would be that when we finally reach a v1.0, we would have an API that wouldn't need to change if we got rid of the C lib and totally replace it with 100% Rust.

I don't know when I might have the time to do that without a sponsor, but that's the idea.

As for removing subscribe_options(), nah. I don't want to break the API without a good reason.

@qm3ster qm3ster force-pushed the subscribe_many_with_options_one branch from 9cb2eb1 to 51bf55e Compare April 23, 2021 00:34
@qm3ster
Copy link
Contributor Author

qm3ster commented Apr 23, 2021

Okay, I moved it to subscribe_many_options plz merge (I also applied many clippy suggestions, almost half of them, in a separate commit)

@fpagliughi
Copy link
Contributor

Oh, cool. I've been meaning to get rid of all those transmute() calls for a while now.

On a quick scroll through, all of those clippy changes look pretty good, but it will take me a while to go through all of them and test. Hopefully I can get to it this weekend.

@qm3ster
Copy link
Contributor Author

qm3ster commented Apr 23, 2021

Oh dang, shot myself in the foot again I have :v
Well alright, take your time.
I did run the existing tests (with mosquitto for broker)

@fpagliughi
Copy link
Contributor

Yeah, I appreciate keeping the clippy stuff in a separate commit, but in general, it's better to keep the whole PR smaller and focused. This isn't one of the more popular Paho libraries (yet!), but there are still thousands of pople/companies using it in production, so we need to be careful with changes so as not to break the code or the API unnecessarily.

@qm3ster qm3ster force-pushed the subscribe_many_with_options_one branch from 51bf55e to 05b8861 Compare April 23, 2021 17:37
@qm3ster
Copy link
Contributor Author

qm3ster commented Apr 23, 2021

Moved clippy changes out of the PR, pls marge :v

@fpagliughi fpagliughi merged commit 059c2f3 into eclipse-paho:master Apr 23, 2021
@qm3ster qm3ster deleted the subscribe_many_with_options_one branch April 24, 2021 16:55
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.

AsyncClient::subscribe_many_with_options loses options if there is only one filter
2 participants