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

Conversation

Projects
None yet
2 participants
@Geod24
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 7, 2015

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 7, 2015

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 7, 2015

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 25, 2015

Contributor

Rebased.

Contributor

Geod24 commented Mar 25, 2015

Rebased.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 25, 2015

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.

Member

s-ludwig 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. 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

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 25, 2015

Contributor

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 ?

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 25, 2015

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 25, 2015

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.

Member

s-ludwig commented Mar 25, 2015

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

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 26, 2015

Contributor

Updated.

Contributor

Geod24 commented Mar 26, 2015

Updated.

registerRestInterface!IAPI(new URLRouter(), new API(), "/root/");
// now IAPI.getInfo is tied to "GET /root/info2"
// Or just to "GET /foo/info2"

This comment has been minimized.

@s-ludwig

s-ludwig Mar 26, 2015

Member

Is the "/root" above ignored when @path is given? I think the URL should be /root/foo/info2 when both are given like here.

@s-ludwig

s-ludwig Mar 26, 2015

Member

Is the "/root" above ignored when @path is given? I think the URL should be /root/foo/info2 when both are given like here.

This comment has been minimized.

@Geod24

Geod24 Mar 26, 2015

Contributor

It is. Comments describe the line under, not above.
So:

  • registerRestInterface!IAPI(new URLRouter(), new API(), "/root/"); => /root/foo/info2
  • registerRestInterface!IAPI(new URLRouter(), new API()); => /foo/info2

But maybe my comments are not clear. I'll rework that.

@Geod24

Geod24 Mar 26, 2015

Contributor

It is. Comments describe the line under, not above.
So:

  • registerRestInterface!IAPI(new URLRouter(), new API(), "/root/"); => /root/foo/info2
  • registerRestInterface!IAPI(new URLRouter(), new API()); => /foo/info2

But maybe my comments are not clear. I'll rework that.

This comment has been minimized.

@s-ludwig

s-ludwig Mar 26, 2015

Member

No sorry, you are right, I've skimmed to fast over this and mistook the following line for something else. Please ignore.

@s-ludwig

s-ludwig Mar 26, 2015

Member

No sorry, you are right, I've skimmed to fast over this and mistook the following line for something else. Please ignore.

[REST] Simplify @path handling
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

This comment has been minimized.

Show comment
Hide comment
@Geod24

Geod24 Mar 26, 2015

Contributor

Update.

Contributor

Geod24 commented Mar 26, 2015

Update.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Mar 27, 2015

Member

Thanks, LGTM!

Member

s-ludwig commented Mar 27, 2015

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Geod24 referenced this pull request Mar 27, 2015

Some cosmetic changes.
- Doc improvements
- Some fixes for examples
- Convert examples to unit tests

@Geod24 Geod24 deleted the Geod24:rootpath-default branch Apr 27, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment