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
Merged

fix #2222 #2223

merged 8 commits into from
Nov 6, 2018

Conversation

deviator
Copy link
Contributor

No description provided.

@deviator
Copy link
Contributor Author

ping @s-ludwig

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

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

LGTM

@deviator
Copy link
Contributor Author

ping @wilzbach

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

Choose a reason for hiding this comment

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

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
Copy link
Member

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

@s-ludwig
Copy link
Member

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

@s-ludwig
Copy link
Member

Merged #2216, which fixes the Meson build error.

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

Choose a reason for hiding this comment

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor Author

deviator commented Nov 6, 2018

ping @wilzbach

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

s-ludwig 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants