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

Figure out a better way to deprecate old feature names #1636

Closed
sgrif opened this issue Apr 12, 2018 · 5 comments
Closed

Figure out a better way to deprecate old feature names #1636

sgrif opened this issue Apr 12, 2018 · 5 comments
Milestone

Comments

@sgrif
Copy link
Member

sgrif commented Apr 12, 2018

The current way we're deprecating the various x-column-tables feature spams builds with an unreasonable number of warnings, and causes our crate to fail to compile if we have #![deny(warnings)]. Ideally we show the warning in the actual invocation of table!, but rust-lang/rust#49912 prevents us from just doing the obvious thing there.

We should find a better solution though, that doesn't spam logs, and in particular, generates no warnings from inside Diesel itself when compiled with these features turned on. The solution of "remember to remove #[deny(warnings)] when we release isn't going to work long term.

@sgrif sgrif added this to the 1.3 milestone Apr 12, 2018
@weiznich weiznich modified the milestones: 1.3, 2.0 Jan 26, 2021
@npapapietro
Copy link
Contributor

What is the status of this with rust-lang/rust#62042 fixing rust-lang/rust#49912

@weiznich
Copy link
Member

weiznich commented Dec 4, 2021

@npapapietro Yes those fix would help here. We are happy to receive a PR that fixes this. Based on the linked rustc issues it should be possible to just add a conditional #[deprecated] attribute to the table! macro. This can be done via #[cfg_attr] based on the deprecated feature flags (large-table, huge-table, ...)

@npapapietro
Copy link
Contributor

I'll try and take this up. As I'm just diving into diesel I'd like some more info as this will be a 1st time pr.

Let me see if if I understand this issue correctly

  • You have a set of features that you would like to deprecate. Where does this list live? I see with-deprecated feature but it is an empty list at the moment.
  • If diesel is build with one of these features we should add the #[deprecated] attr to the table! macro. Would this be injected into the $meta macro arg in __diesel_parse_table or at the impl of each feature case? This is where I am slightly confused.

@weiznich
Copy link
Member

weiznich commented Dec 5, 2021

You have a set of features that you would like to deprecate. Where does this list live? I see with-deprecated feature but it is an empty list at the moment.

We would like to deprecate the huge-tables feature in favor of the 64-column-tables feature flag. Also the large-tables feature in favor of the 32-column-tables feature.

If diesel is build with one of these features we should add the #[deprecated] attr to the table! macro. Would this be injected into the $meta macro arg in __diesel_parse_table or at the impl of each feature case? This is where I am slightly confused

Adding the deprecated attribute inside the expanded table! macro does not work, as code in those context does not have access to the corresponding flag. (That's because the expanded code is parsed in the context of a third party crate.)
I propose that we just add the the #[deprecated] attribute on table! itself, so that the user gets the corresponding warning there. The deprecated attribute should be only conditionally enabled via #[cfg_attr].

@weiznich
Copy link
Member

weiznich commented Dec 7, 2021

Closed by #2968

@weiznich weiznich closed this as completed Dec 7, 2021
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

No branches or pull requests

3 participants