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

Simplify @path handling #999

Merged
merged 1 commit into from
Mar 27, 2015
Merged

Simplify @path handling #999

merged 1 commit into from
Mar 27, 2015

Conversation

Geod24
Copy link
Contributor

@Geod24 Geod24 commented Mar 7, 2015

  • Make @rootPathAttribute the default: it just scales better, there is rarely a point in having multiples interfaces at the root.
  • Kill RootPathAttribute and use PathAttribute only instead: There was no real difference between @rootpath and @path, as they were mutually exclusive (path on method, rootPath on interface) and had the same behavior (e.g. they are always relative). This will obviously make user code less error-prone.
  • Communicate the settings to the sub-interface.

@Geod24
Copy link
Contributor Author

Geod24 commented Mar 7, 2015

Ping @Dicebot as we discussed it.
@s-ludwig : Why is RestInterfaceSettings a class ? Shouldn't it be a struct ?

@s-ludwig
Copy link
Member

s-ludwig commented Mar 7, 2015

Make @rootPathAttribute the default: it just scales better, there is rarely a point in having multiples interfaces at the root.

You mean @rootPathFromName? I don't agree here. This would be a silent breaking change and for what it's worth, all of my web interfaces currently have the default path of "/". We can't make this change like this without a deprecation path, but I also think that making the path inference explicit is the better idea. It just doesn't seem obvious to me that the type name would automatically become part of the path. Is that possibly different for people coming from certain other web frameworks?

Deprecation of @rootPath sounds fine, though.

Why is RestInterfaceSettings a class ? Shouldn't it be a struct ?

It's just to be consistent with all other *Settings types in the library (I think that HTTPServerSettings was the first - not really sure why I made it a class at the time). There are some low priority pros and cons for either side, but in the end none of them matters much, so I wouldn't want to go through the hassle and change everything now.

@Geod24
Copy link
Contributor Author

Geod24 commented Mar 7, 2015

You mean @rootPathFromName? I don't agree here. This would be a silent breaking change and for what it's worth, all of my web interfaces currently have the default path of "/". We can't make this change like this without a deprecation path, but I also think that making the path inference explicit is the better idea. It just doesn't seem obvious to me that the type name would automatically become part of the path. Is that possibly different for people coming from certain other web frameworks?

Yes, I meant @rootPathFromName.
I'm aware of the breaking change, and that's also a problem I considered. I couldn't find any "perfect" solution, beside deprecating un-marked interface in one release, then removing @rootPathFromName in the next, before deprecating it few releases later (but it's rather annoying on a user POV, but probably better).

As per why I want it to be the default :
The REST code generator is, ATM, a kind of gloryfied RPC system IMO.
Usually when you have a REST interface (I think Github's one of the best example), you have 2 kinds of "objects": a ressource, and a collection. Examples of collections are /users/, /posts/ (for a blog), /comments/, /organizationName/ (in case of Github). In those collections, you must have ressources (OFC your collection can be empty).
However, in vibe.web.rest, such construction is neither enforced, nor "easy" to do. It's doable, but there's a lot of gotchas. One of them is @rootPathFromName, as it's an opt-in strategy to this kind of design.

Again, another possibility is to deprecate un-annotated interface for X releases / X time.

It's just to be consistent with all other *Settings types in the library [...]

Ok, makes sense.

@Geod24
Copy link
Contributor Author

Geod24 commented Mar 25, 2015

Rebased.

@s-ludwig
Copy link
Member

You are right that collection/object handling still needs to be improved. I had some ideas for that, but have to think about this again to remember the details. But please let's leave this as a separate concern for now, because deprecating @rootPath is good and doesn't really depend on the by-name change.

One thing regarding @rootPath: before marking it as deprecated it should just get a doc comment "Scheduled for deprecation - please use $(D @path) instead.(I need to get that written down somewhere). Directly going todeprecated` is always annoying during the transition phase from one version to another.

@Geod24 Geod24 force-pushed the rootpath-default branch from 0310b31 to 9fd5283 Compare March 25, 2015 21:00
@Geod24
Copy link
Contributor Author

Geod24 commented Mar 25, 2015

You are right that collection/object handling still needs to be improved. I had some ideas for that, but have to think about this again to remember the details.

I'd be interested to know, when you have time to write them down (unrelated: did you get my email?).

For the deprecation, I agree, I just didn't like the double-deprecation (we ask to use rootPathAttribute, then we remove it).

Is this new version better ?

@Geod24 Geod24 force-pushed the rootpath-default branch from 9fd5283 to cbdb129 Compare March 25, 2015 21:39
@Geod24
Copy link
Contributor Author

Geod24 commented Mar 25, 2015

One thing regarding @rootPath: before marking it as deprecated it should just get a doc comment "Scheduled for deprecation - please use $(D @path) instead." (I need to get that written down somewhere). Directly going to deprecated is always annoying during the transition phase from one version to another.

I'm not sure I understand your point. The point of deprecated is to have a nice way to tell your users that something is going to break. Most users won't bother to look the doc up on each release, especially something as basic as `@rootPath.

This policy is similar to what Druntime/Phobos use to do. But now they're going straight to deprecated because you can give a message with deprecated.

@s-ludwig
Copy link
Member

Is this new version better ?

What I meant was that I'd like to exclude the @rootPathFromName change from the PR until we have reached a final conclusion. We can get the @path<->@rootPath change in now, though (with the deprecated <-> "scheduled for deprecation" change).

I'm not sure I understand your point.

A typical situation is that you have a project with dependencies to several libraries. Now it may easily be the case that library A only works with vibe.d 0.7.22, but library B requires 0.7.23. If we now immediately deprecate something in 0.7.23, there is no way to get a clean build with up-to-date dependencies. Including an additional "soft deprecation" step at least alleviates this issue.

@Geod24 Geod24 force-pushed the rootpath-default branch 2 times, most recently from 0352b61 to 27eff74 Compare March 26, 2015 00:27
@Geod24
Copy link
Contributor Author

Geod24 commented Mar 26, 2015

Updated.

release (0.7.25), and later turned into an error (0.7.26) to allow
users to catch any unattributed interface and annotate them with
@path("/"). Later on (0.7.27), the unattributed interfaces will be
legal again, with a default path generated from their name.
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph should get removed for this PR.

Ultimately, we want to make @rootPathAttribute the default for interface (it's already the default behavior for methods), and remove the @rootpath attribute, because it's redundant with @path, and they currently don't overlap, @rootpath being dedicated to interfaces, and @path to method.

This commit:
- Document future deprecation of @rootpath and redefine @rootpath and @rootPathFromName in terms of @path;
- Documents @path better;
- Communicate the settings to the sub-interface;
@Geod24 Geod24 force-pushed the rootpath-default branch from 27eff74 to 7508df2 Compare March 26, 2015 22:52
@Geod24
Copy link
Contributor Author

Geod24 commented Mar 26, 2015

Update.

@s-ludwig
Copy link
Member

Thanks, LGTM!

s-ludwig added a commit that referenced this pull request Mar 27, 2015
@s-ludwig s-ludwig merged commit b0b6a30 into vibe-d:master Mar 27, 2015
Geod24 referenced this pull request Mar 27, 2015
- Doc improvements
- Some fixes for examples
- Convert examples to unit tests
@Geod24 Geod24 deleted the rootpath-default branch April 27, 2015 16:47
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