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

UDA support for REST interfaces and rest module cleanup #189

Merged
merged 4 commits into from
Mar 7, 2013
Merged

UDA support for REST interfaces and rest module cleanup #189

merged 4 commits into from
Mar 7, 2013

Conversation

mihails-strasuns
Copy link
Contributor

Issue #188

Module structure changes:
    1) Added vibe.http.restutil module that contains HTTP/REST-agnostic meta-programming utilities
    2) Moved all matching function/templates from rest to restutil

Features:
    1) Implemented UDA support for REST interfaces - @method and @path

Rework;
    1) Adjusted formatting in vibe.http.rest to single most prevailing style
    2) Reworked all code generation functions to use token strings + format
    3) Replaced copy-paste with table lookup in extractHttpMethodAndName

.. and various minor tweaks I have probably done subconsciously.

Issues:
1)I know close to nothing about initial vibe.d setup and thus failed to adjust functional tests so that server is properly run. Please take a look at tests folder.
2) Update is probably worth adding separate article on REST in main vibe.d documentation. I have not yet done it.

Other than that, please, destroy. One controversial part is that pragma printing of generated methods is not that pretty know, I have decided that it is less important than having pretty main source code.

@mihails-strasuns mihails-strasuns mentioned this pull request Mar 4, 2013
@BitPuffin
Copy link

Looks promising! We'll need to see what @s-ludwig thinks. It should be as clean and adaptable as possible so Vibe doesn't run in to the same wall later on and once again needs to change how the REST generator works

}

/// private
template getSymbols(T)
struct Path
Copy link
Member

Choose a reason for hiding this comment

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

I dont't think that Path is a good idea. This is highly likely to break all client code that uses vibe.inet.path.Path and annoyingly means that the module path has to be typed out from now on. Maybe PathTag or PathOverride and I think it could even be private (although that unfortunately wouldn't help with the symbol conflict for external code).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, beg my pardon, completely forgot to change it after our last discussion on topic as it "just worked" at the moment. My call is for "OverridenPath", as it is data structure, not UDA itself.

@s-ludwig
Copy link
Member

s-ludwig commented Mar 5, 2013

In general I think this looks good (apart from the struct name issue) and I would tend to merge it an check with my existing projects for any possible problems(*). It's a bit unfortunate that refactoring+style changes+implementation chages are conflated in one commit as it makes the diff really hard to read, but I think that I got everything.

Regarding the code style, I severely need to take some time to finish the code conventions and put them in the wiki or something, as this problem came up quite often recently. In this particular case, the style has departed a bit from the rest of the library, but until the code conventions are up and finished this is ok and at least the module is self-consistent.

But the reformatted q{} strings + format() really do a great job regarding readability. I'm not sure about the memory consumption implications of format during compile time, but I would assume that this is a non-issue for typically sized interfaces (in contrast to the Diet template compiler, where every bit of saved memory is important).

The repeated regex replacements could probably be replaced by a single call to format(), but that's a minor optimization that can be delayed until it shows that there actually is a performance problem (due to memory allocations).

Thanks again for your efforts!

(*) actually I guess I could just check out your branch and check before merging...

@s-ludwig
Copy link
Member

s-ludwig commented Mar 5, 2013

@BitPuffin: I wouldn't say that we really hit a wall here. It's just that UDAs have enabled unforseen possibilities here so that the code is suddenly able to do much more than what was originally indended. But I agree that we should be careful here. However, in this case I'm quite confident that the annotations are basic enough that they won't interfere with future additions

@mihails-strasuns
Copy link
Contributor Author

@s-ludwig

Well, as soon as someone will start using REST interfaces that huge, that shouldn't be problem to optimize :) For now compile time is negligible for me, so I was not even thinking about performance in CTFE.

Regarding squashed commits, there was a problem in original commit history that I was learning how stuff works parallel with implementing new one and thus was changing my mind all the way around, there were dozens of commits with same blocks of code moved here and back, features added and removed and so on. I though about splitting to meaningful commits after squashing but overall change distribution felt like it is just easier to read new version as a whole than track separate changes.

Can do splitting if you need it for better git history though.

@s-ludwig
Copy link
Member

s-ludwig commented Mar 5, 2013

No worries with the history, although it would be better to have it split up into three commits, I think it's not important enough to warrant more work [except if it is trivial ;)].

The CTFE stuff for me is less about performance, but more about maximum memory usage of DMD. But even then I'd hope that the amount of memory is small enough here (but sometimes it can be surprising how much the memory usage piles up when processing a seemingly small string at CT).

Dicebot added 4 commits March 7, 2013 12:27
…pection and

metaprogramming tools not directly related to HTTP or REST.

New function "cloneFunction" is provided with implementation partially copied from
current Phobos fullyQualifiedName, it is a replacement for bunch of functions used
for the same purprose in vibe.http.rest

Provided easy migration path to next dmd version with switching to Phobos
fullyQualifiedName
"generateRestInterfaceMethods" reworked to use toekn strings + format
…ses tests now.

Removed functions made redundant by vibe.http.restutil
Moved rest of code gen to token strings
Changed formatting and naming conventions to more consistent ones
@mihails-strasuns
Copy link
Contributor Author

Have split commits, fixed UDA struct names and some whitespace issues.
Anything else?

@s-ludwig
Copy link
Member

s-ludwig commented Mar 7, 2013

Thanks a lot! I don't see anything missing. Will merge and test in a few hours after I fixed some breakage...

s-ludwig added a commit that referenced this pull request Mar 7, 2013
UDA support for REST interfaces and rest module cleanup
@s-ludwig s-ludwig merged commit c0aa72d into vibe-d:master Mar 7, 2013
s-ludwig added a commit that referenced this pull request Mar 8, 2013
…artial fix up of #189.

Still relies on a fix in packageName!() and sub interfaces are not working.
s-ludwig added a commit that referenced this pull request Mar 8, 2013
…ges of #189).

Everything compiles now on stock DMD 2.061, including sub interfaces.
@s-ludwig
Copy link
Member

s-ludwig commented Mar 8, 2013

It turned out that there were quite some things not compiling. Did you compile this on a customized DMD/Phobos or did things break during the history rewrite?

It compiles again now, but have yet to test it at runtime.

@mihails-strasuns
Copy link
Contributor Author

@s-ludwig Some part of functionality may have not been instantiated in my code probably, there are templates everywhere after all. Or I simply got sloppy and used wrong folder when running tests after commit split. My bad : (

@mihails-strasuns
Copy link
Contributor Author

@s-ludwig btw, I have sent you an e-mail via [email protected], hope this address is alive.

@mihails-strasuns mihails-strasuns deleted the uda-rest branch March 9, 2013 11:27
@s-ludwig
Copy link
Member

s-ludwig commented Mar 9, 2013

Got it. No problem regarding the breakage, such things happen fast with templates, I know. At some point I'd like to have at least an automatic pull tester, like the one osed for D-Programming-Language. So there seems to be also some runtime breakage, but I'll fix that now.

@mihails-strasuns
Copy link
Contributor Author

P.S. I have noticed that you have not checked added test for restclient (in /tests/source/tests/restclient.d), it still fails with "[7FB4481B60B4:7FB4481BEBB4 ERR] Task terminated with exception: Failed to connect to host 127.0.0.1 on port 8000: 33" for me for reason I can't understand.

@s-ludwig
Copy link
Member

Okay, everything is properly fixed up now. I added HttpServerSettings.disableDistHost so that listenHttp can be run in a mode where it immediately starts to listen and put all tests in a task so that they can all run backed by the normal event loop.

@s-ludwig
Copy link
Member

Sorry, forgot to explain the issue. So the problem was that listenHttp by default waits for processCommandlineArgs() to be called first, so that it can do some vibedist setup before listening, if required. In the tests project, processCommandLineArgs() was never called, so it failed. By setting HttpServerSettings.disableDistHost it will now immediately listen, so that even the timer is not required anymore.

This corner of the API still needs some beutification, once it is clear how the whole vibedist thing will be working in the end.

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.

3 participants