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

Remove some deprecated code #2475

Merged
merged 2 commits into from Nov 15, 2020
Merged

Remove some deprecated code #2475

merged 2 commits into from Nov 15, 2020

Conversation

Geod24
Copy link
Contributor

@Geod24 Geod24 commented Sep 6, 2020

The alias this deprecation in particular is quite painful.

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

Though this shouldn't be merged until we decide on whether there will be a stable branch (in some form) or releases will continue from master.

@s-ludwig
Copy link
Member

Needs a .byKeyValue:

http/vibe/http/client.d(297,4): Error: invalid foreach aggregate res.headers, define opApply(), range primitives, or use .tupleof

I'm not quite there yet with having a strict stable branch policy (one example being the slightly breaking change in #2487), so this is fine to merge (but it would be anyway, since this is master). We/I should prepare a wiki page with parts of the API that need to be reworked. Once the low-hanging fruits are done and vibe-http is properly migrated to the new repository, I'd be open to switch to a more formal scheme, even jumping to 1.0.0 could be an option then, deferring the rest of the breaking changes to 2.0.0.

@Geod24
Copy link
Contributor Author

Geod24 commented Oct 27, 2020

Would love to see this!

Regarding this PR, I'm not so sure removing the alias this and expecting people to use byKeyValue was a good idea.
Perhaps providing range primitives would be better suited here, wouldn't it ?

@s-ludwig
Copy link
Member

The really bad thing is that there is no cue in the deprecation warning showing where in the code alias this is being used, so that although the deprecation has been there for a long time now, some people may not yet have fixed the issue. Maybe letting alias this point to a template method with a static assert(false) and a migration note could mitigate that.

How would you provide range primitives other than with an alias this, though?

@s-ludwig
Copy link
Member

I've created a wiki page and added all actionable high-level items that I could find from a quick look: https://github.com/vibe-d/vibe.d/wiki/API-Improvements -- there may be some low level issues that I missed now, but most of them are supposedly in the modules that should be moved out of this repository anyway.

@Geod24
Copy link
Contributor Author

Geod24 commented Oct 27, 2020

The really bad thing is that there is no cue in the deprecation warning showing where in the code alias this is being used, so that although the deprecation has been there for a long time now, some people may not yet have fixed the issue. Maybe letting alias this point to a template method with a static assert(false) and a migration note could mitigate that.

Funny you would mention this: dlang/dmd#11839
It will be part of v2.095.0, but you can already use it to find the origin of those deprecation messages.

How would you provide range primitives other than with an alias this, though?

struct Bar { int v; }
void main ()
{
    Bar b;
    foreach (x; b) {}
}
$ dmd foo.d
foo.d(5): Error: invalid foreach aggregate b, define opApply(), range primitives, or use .tupleof

In this case, I think range primitives would make most sense (front / empty / popFront).

@s-ludwig
Copy link
Member

Okay, but that's what .byKeyValue provides and the alias this makes it available for DictionaryList itself. Having it on DIctionaryList directly wouldn't make sense obviously.

But the problem with alias this and range primitives is that it restricts the iteration to a single definition (key+value in this case) and to me it feels much cleaner to always provide explicit range properties instead. opApply was more flexible in that regard, but the complete inability to handle attributes (e.g. nothrow and @safe) properly makes that a non-option.

@Geod24
Copy link
Contributor Author

Geod24 commented Oct 28, 2020

a single definition (key+value in this case)

Using range primitives you can actually expose only value and rely on enumerate, but it's extra work.

but the complete inability to handle attributes (e.g. nothrow and @safe) properly makes that a non-option.

You might be interested in my DConf talk then ;)

I don't think I'll be able to come back to this PR this week. I have some deadlines at work, plus the DConf talk which limit my time quite greatly ATM.

@s-ludwig
Copy link
Member

Using range primitives you can actually expose only value and rely on enumerate, but it's extra work.

Not for a key-value-map, like in this case, though ;-)

You might be interested in my DConf talk then ;)

Indeed!

@s-ludwig
Copy link
Member

There is still another use of alias this in one of the examples:

+cd examples/http_info
+dub build --compiler=dmd --build-mode=separate
(...)
info.dt(10,2): Error: invalid foreach aggregate req.headers, define opApply(), range primitives, or use .tupleof
info.dt(31,2): Error: invalid foreach aggregate req.query(), define opApply(), range primitives, or use .tupleof
info.dt(37,2): Error: invalid foreach aggregate req.form(), define opApply(), range primitives, or use .tupleof

@s-ludwig s-ludwig mentioned this pull request Nov 14, 2020
@Geod24 Geod24 force-pushed the deprecated branch 2 times, most recently from 90aa415 to 5a120be Compare November 14, 2020 13:57
This deprecation message unfortunately creeps up in user's code,
without the user having a possibility to solve it.
As the message suggests, they should have been removed already.
@Geod24
Copy link
Contributor Author

Geod24 commented Nov 14, 2020

Should be (finally) G2G

@s-ludwig s-ludwig merged commit 6068b17 into vibe-d:master Nov 15, 2020
2 checks passed
@Geod24 Geod24 deleted the deprecated branch November 15, 2020 09:58
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

3 participants