jeikabu / nng-rust

MIT License
18 stars 8 forks source link

remove `build-nng` from default features #33

Open M1cha opened 3 years ago

neachdainn commented 3 years ago

I'll assume the commit message is the message you intended for this pull request.

Disabling default features is pretty hard though because every single crate that depends on nng-sys would have to use default-features = false.

That is quite unrealistic - especially since non-binary crates usually don't care about this.

As far as I can tell, there is really only a single published crate that directly depends on nng-sys and that crate does use default-features = false while providing its own feature for building the library. This to me suggests that either nng-rs is a direct dependency of the final binary or any layers between the binary and nng-sys are internal. In the former case, this just changes a trivial default that will not genuinely impact anything downstream of nng-rs. In the latter case, requiring that single change in internal libraries is very realistic.

I'm open to seeing some numbers (anecdotal or not) that contradict this, but for the time being I am unconvinced this is a good change. Particularly since I heavily utilize this feature in my own work.

M1cha commented 3 years ago

I'll assume the commit message is the message you intended for this pull request.

IMO there's no point in copying the message of the commit into the PR. It's duplicate info and doesn't get updated automatically when I change the commit. Also I might have multiple commits. Also, the change is where the description should be, not the merge-commit(which might not exist if you use rebase-merge).

Yes I'm using nng-rs but it does not use default-features = false.

In general, yes there's only one crate right now which uses nng-sys but that shouldn't affect the decision of it's design at all given it's a public crate that could be used anywhere.

neachdainn commented 3 years ago

IMO there's no point in copying the message of the commit into the PR. It's duplicate info and doesn't get updated automatically when I change the commit. Also I might have multiple commits.

It should provide a summary of the changes you're asking to merge, if for no other reason than to be polite. If the merge request was any larger, I would probably put it on the back burner simply because I don't want to have to dig just to get a summary of what is going on.

Yes I'm using nng-rs but it does not use default-features = false.

You're right. Apparently I messed that up when I upgraded nng-sys versions. I'll have to fix that.

Overall, there are more significant issues with this crate that prevent this change from being meaningful. The versioning scheme alone is causing problems (@jeikabu I will probably switch it - we can discuss). Additionally, the primary consumer of the crate completely negates this change anyway.

That being said, if you can fix the CI and open a similar MR to nng-rs, I'll consider merging it. Things need to be fixed anyway.