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

Cleanup #396

Merged
merged 3 commits into from
Jun 8, 2022
Merged

Cleanup #396

merged 3 commits into from
Jun 8, 2022

Conversation

EdJoPaTo
Copy link
Contributor

@EdJoPaTo EdJoPaTo commented May 14, 2022

Listed changes from the commits:

  • Cargo.toml dependencies:
    • sorted alphabetically
    • use shorter version identifier with same meaning
  • remove unused test variables
  • add ; at last statement
  • cargo fmt
  • same mqttoptions variable name in all examples
  • minor things found in Improve MqttOptions creation #391

This should also simplify the diff of #391 (this will create merge conflicts in #391 first)

- sorted alphabetically
- use shorter version identifier with same meaning
- remove unused test variables
- add ; at last statement
- cargo fmt
- same mqttoptions variable name in all examples
- minor things found in #391
@EdJoPaTo
Copy link
Contributor Author

Anything blocking this? (Having not that much time to review is also understandable and totally valid)

@tekjar
Copy link
Contributor

tekjar commented May 24, 2022

This mostly looks good to me. I prefer to freeze versions in libraries as well, atleast critical crates like tokio because I notices some upgrades causing perf bottlenecks in the past

@EdJoPaTo
Copy link
Contributor Author

EdJoPaTo commented May 25, 2022

I prefer to freeze versions in libraries as well, atleast critical crates like tokio because I notices some upgrades causing perf bottlenecks in the past

The version identifiers which changed in Cargo.toml have exactly the same meaning before and after the change. Both 1.2 and 1 will install version 1.33.7 (when without the lock file). This is counterintuitive for me so I like the shorter version more.
The lock file is still in place and unchanged.

A different idea might be to replace the version requirements with a different strategy like tilde requirements and remove the lock file then.

Should I revert that change in version requirements in order discuss it differently from this PR?

@tekjar
Copy link
Contributor

tekjar commented May 25, 2022

@EdJoPaTo Oh yeah. Probably just use =1.33.7 for tokio? I'm only worried about the movement of tokio's version

@EdJoPaTo
Copy link
Contributor Author

Remove the lock file then too when the tokio version is pinned?

@tekjar
Copy link
Contributor

tekjar commented May 25, 2022

I think that lock file is related to some binary in this workspace. I think it's all cool w.r.t this library. I'll merge this tomorrow in our opensource syncup

@de-sh de-sh self-requested a review June 6, 2022 14:20
@de-sh de-sh merged commit bef47f5 into bytebeamio:master Jun 8, 2022
@de-sh de-sh mentioned this pull request Jun 8, 2022
Closed
@EdJoPaTo EdJoPaTo deleted the cleanup branch June 16, 2022 20:00
@adamscybot
Copy link

adamscybot commented Jun 20, 2022

So the above caused some problems for me. I have another library which uses tokio transitively (dep of a dep) and since it requires 1.19, and rumqttc is now strict about using 1.17.0 -- I cant use the library. Previously, it was just at 1 which means it would work with other versions of tokio that may be required by other libs.

I suspect I'm probably not alone since tokio is very popular so in a complex app with lots of dependencies there's a very high chance usage of this library is now precluded with such a strict req.

@EdJoPaTo
Copy link
Contributor Author

Thanks @adamscybot for bringing it up!
I think this is its own topic that might be bigger than this PR. I created an issue for discussing it in its own place.

carlocorradini pushed a commit to carlocorradini/rumqtt that referenced this pull request Aug 3, 2023
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.

None yet

4 participants