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

fix #2222 #2223

Merged
merged 8 commits into from Nov 6, 2018

Conversation

Projects
None yet
3 participants
@deviator
Copy link
Contributor

commented Oct 20, 2018

No description provided.

deviator added some commits Oct 19, 2018

@deviator

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2018

ping @s-ludwig

@wilzbach
Copy link
Member

left a comment

LGTM

@deviator

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2018

ping @wilzbach

dst.put(":");
if (isDoubleSlashSchema(schema))
dst.put("//");
}

This comment has been minimized.

Copy link
@s-ludwig

s-ludwig Oct 25, 2018

Member

Without the schema, the generated string is not a URL anymore. Of course it could be argued to extend this class into a full URI, but since this is just a side-effect, I'd like to keep that a separate question. Without having looked at the JS generation code too closely, to me it seems like this optimization should be handled there. If I'm not mistaken, this should also be made an opt-in/out setting, since the JS might be accessed from page hosted on a different server or sub domain.

@wilzbach

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

Travis failures are very likely related to vibe-d/eventcore#86

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

@wilzbach, I only see std.digest related errors and the Meson build failing

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

Merged #2216, which fixes the Meson build error.

fout.put(" + ");
fmtParam(fout, p);
}
}

This comment has been minimized.

Copy link
@s-ludwig

s-ludwig Oct 26, 2018

Member

Would be good to have an assert(false, "Empty route function path"); here in the else case, so that no invalid JavaScript can be produced here. (the else if could also be merged to a single line without an extra block)

else fout.formattedWrite("toRestString(%s)", p.text);

// if url not empty
if (intf.baseURL != intf.basePath) {

This comment has been minimized.

Copy link
@s-ludwig

s-ludwig Oct 26, 2018

Member

I think using intf.baseURL != URL.init would make this a bit more obvious here.

This comment has been minimized.

Copy link
@deviator

deviator Oct 26, 2018

Author Contributor

intf.baseURL is a string, not URL. URL.init is empty, baseURL == basePath can be if URL.init is passed to RestInterface.ctor here
I don't understand why RestInterface use strings for baseURL and basePath instead URL for both. Is it principled? Why URL can't be initialize with empty string? Why URL.init prints not empty string? Maybe it can be changed? I think it can simplify code.

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

Otherwise, apart from the failures looks good to merge now to me too. Would also be good to make a final squash/rebase to get rid of the two merge commits.

@wilzbach

This comment has been minimized.

Copy link
Member

commented Oct 26, 2018

@wilzbach, I only see std.digest related errors and the Meson build failing

Look at the vibe-core failures:

+cd tests/mongodb
+dub --compiler=dmd --override-config=vibe-d:core/vibe-core --build-mode=separate
Performing "debug" build using dmd for x86_64.
diet-ng 1.5.0: target for configuration "library" is up to date.
mir-linux-kernel 1.0.1: target for configuration "library" is up to date.
taggedalgebraic 0.10.12: target for configuration "library" is up to date.
eventcore 0.8.37: target for configuration "epoll" is up to date.
stdx-allocator 2.77.4: target for configuration "library" is up to date.
vibe-core 1.4.3: target for configuration "epoll" is up to date.
vibe-d:utils 0.8.4+commit.23.gce16c4a: target for configuration "library" is up to date.
vibe-d:data 0.8.4+commit.23.gce16c4a: target for configuration "library" is up to date.
vibe-d:crypto 0.8.4+commit.23.gce16c4a: target for configuration "library" is up to date.
vibe-d:stream 0.8.4+commit.23.gce16c4a: target for configuration "library" is up to date.
vibe-d:textfilter 0.8.4+commit.23.gce16c4a: target for configuration "library" is up to date.
vibe-d:inet 0.8.4+commit.23.gce16c4a: target for configuration "library" is up to date.
vibe-d:tls 0.8.4+commit.23.gce16c4a: target for configuration "openssl" is up to date.
vibe-d:http 0.8.4+commit.23.gce16c4a: target for configuration "library" is up to date.
vibe-d:mongodb 0.8.4+commit.23.gce16c4a: target for configuration "library" is up to date.
tests ~master: building configuration "application"...
Linking...
To force a rebuild of up-to-date targets, run again with --force.
Running ./tests 
Program exited with code -11

Similar errors for vibe.d with the vibe-core subconfiguration happen on the Project Tester since yesterday, which is why I think it's due to vibe-d/eventcore#86

@deviator

This comment has been minimized.

Copy link
Contributor Author

commented Nov 6, 2018

ping @wilzbach

@s-ludwig s-ludwig merged commit 8b0efd5 into vibe-d:master Nov 6, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@s-ludwig

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

The remaining failures were just due to stdx-allocator still failing for the Meson build (I guess it just requires a new version tag).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.