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

Added VibeDataNoOpDispatch version #1526

Merged
merged 3 commits into from
Jun 23, 2016

Conversation

dmonagle
Copy link
Contributor

This should not break existing code but allows us to define a version VibeDataNoOpDispatch. This prevents the compilation of the opDispatch functions in both Bson and Json.

We are semi-regularly running into issues with people using UFCS on a Json object. Instead of calling the function, it uses opDispatch and more often than not returns Json.undefined. This compiles without a problem and sometimes drives you mad when tests fail and you can't see the (not so) obvious issue.

Considering they are deprecated anyway, this allows a nice way for people to throw away this functionality immediately and make better use of things like UFCS.

@s-ludwig
Copy link
Member

I just noticed a few days ago that DMD doesn't even evaluate the deprecated attribute for opDispatch (anymore?), which is really unfortunate. The best thing that I can see for continuing the deprecation path would be to put a pragma(msg) inside. Would that also be sufficient in your case? But I wouldn't mind adding the version block either.

@dmonagle
Copy link
Contributor Author

We noticed the lack of deprecated message as well. While a pragma message might be handy, I know we have been ready for a long time to ditch the functions as they are a little dangerous.

For example, In our controllers we have some functions like validateResponse(Json). We have had to remember to call them by calling validateReponse(jsonPayload) but every now and again somebody will accidentally call jsonPayload.validateResponse - which compiles. We'd love to see the version block included as it's a big pain point.

I've also added a second commit as there were a few spots in the vibe code that were still relying on opDispatch.

@s-ludwig
Copy link
Member

Okay, I'll merge this when Travis is done and will then add the pragma afterwards.

@s-ludwig s-ludwig merged commit 2666027 into vibe-d:master Jun 23, 2016
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