Skip to content

Conversation

@weiznich
Copy link
Contributor

During a dependency review we noticed that the displaydoc crate includes various development scripts. These development scripts shouldn't be there as they might, at some point become problematic. As of now they prevent any downstream user from enabling the [bans.build.interpreted] option of cargo deny.

I opted for using an explicit include list instead of an exclude list to prevent these files from being included in the published packages to make sure that everything that's included is an conscious choice.

Copy link
Collaborator

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Please, please, stop making these PRs without including tests/examples/etc.

Perhaps just use explicit excludes instead? Makes it clearer to the reviewers what the problem files are, instead of foisting the burden on the reviewer to do this.

@weiznich
Copy link
Contributor Author

Please, please, stop making these PRs without including tests/examples/etc.

I would again highlight that this is personal preference whether to include or exclude examples. I accept that you want them to be there, but I just don't know in which crates your are currently involved and in which not.
Other reviewers are more than happy to remove them, so there is really no reason to be so harsh about that.

Makes it clearer to the reviewers what the problem files are, instead of foisting the burden on the reviewer to do this.

I'm happy to provide a list of these files if necessary:

For displaydoc it's the update-readme.sh file in the crate root.

During a dependency review we noticed that the displaydoc crate includes various development scripts. These development scripts shouldn't be there as they might, at some point become problematic. As of now they prevent any downstream user from enabling the `[bans.build.interpreted]` option of cargo deny.

I opted for using an explicit include list instead of an exclude list to prevent these files from being included in the published packages to make sure that everything that's included is an conscious choice.
@weiznich
Copy link
Contributor Author

Adjusted the PR to include tests as requested (no examples or benchmarks exist for this crate)

@Manishearth
Copy link
Collaborator

Manishearth commented Dec 16, 2025

I would again highlight that this is personal preference whether to include or exclude examples.

Yes, I'm well aware that there are conflicting pressures to include/exclude tests: as a maintainer I get asked to go both ways on the semi-regular.

But you don't need to think about that in these PRs: that personal preference is likely to be reflected in the current value of the include/exclude key, which you can check when making these PRs. Whatever the reviewer preference may actually be, it would not be controversial to keep the status quo, and if these PRs is how reviewers discover they have a preference counter to the status quo, that's great, that's their problem.

I think it's misleading to make PRs that say that they exclude development scripts when in practice they lead to a lot of other things being excluded. It's harder on the reviewer since the reviewer actually has to go check, and not all reviewers will remember to do this. It's not even clear exactly what files are problematic from these PRs, e.g. I'm super wary about rust-fuzz/libfuzzer#137 because it's a native build and I don't want to break it, but also I don't actually know what the problem files are.

@Manishearth Manishearth merged commit ff03f57 into yaahc:master Dec 16, 2025
8 checks passed
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