Skip to content

Remove <base> tag, use hrefs prepended with base #2098

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

Merged
merged 5 commits into from
Jan 7, 2020

Conversation

jdkoren
Copy link
Contributor

@jdkoren jdkoren commented Dec 17, 2019

For technical reasons, we can't really compute and inline the href base when accessing a model's href, so instead we use a placeholder and substitute it just after rendering each template.

See discussion in #2090

@googlebot googlebot added the cla: yes Google CLA check succeeded. label Dec 17, 2019
@@ -14,6 +14,10 @@ import 'package:pub_semver/pub_semver.dart';

final RegExp substituteNameVersion = RegExp(r'%([bnv])%');

// Unlikely to be mistaken for an identifier, html tag, or something else that
// might reasonably exist normally.
final String HTMLBASE_PLACEHOLDER = '\%\%HTMLBASE\%\%';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just off the top of my head, happy to replace it with something else.
The backslash escapes are unnecessary, but should prevent this string from being substituted if someone runs Dartdoc on Dartdoc itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

This string is short enough I can imagine someone else using it reasonably. Maybe: '\%\%__HTMLBASE_dartdoc_internal_only\%\%' instead?

Copy link
Contributor

@jcollins-g jcollins-g left a comment

Choose a reason for hiding this comment

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

This in general looks OK.

I'd like to see that a test verifies behavior with manually constructed links to elements using the old and new assumptions.

@jdkoren
Copy link
Contributor Author

jdkoren commented Jan 3, 2020

This in general looks OK.

I'd like to see that a test verifies behavior with manually constructed links to elements using the old and new assumptions.

I added two tests to model_test, but they only check that the relative hrefs in the doc comment is left unchanged; there isn't really a way to verify that one of those is actually a broken link (Dartdoc does emit a brokenLink warning for it though). Let me know if you have other thoughts on what we could look for here.

Copy link
Contributor

@jcollins-g jcollins-g left a comment

Choose a reason for hiding this comment

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

Alright, this is looking doable, even if the implementation is more than a bit horrifying in practice. I would like to see this behavior controllable via a flag (OK with default-on, we'll just shut it off anywhere it causes a problem if need be until we can fix downstream users). I also want a big TODO to lock down TemplateData as I discussed in the original bug. But I'm otherwise willing to tolerate it in the meantime.

@@ -14,6 +14,10 @@ import 'package:pub_semver/pub_semver.dart';

final RegExp substituteNameVersion = RegExp(r'%([bnv])%');

// Unlikely to be mistaken for an identifier, html tag, or something else that
// might reasonably exist normally.
final String HTMLBASE_PLACEHOLDER = '\%\%HTMLBASE\%\%';
Copy link
Contributor

Choose a reason for hiding this comment

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

This string is short enough I can imagine someone else using it reasonably. Maybe: '\%\%__HTMLBASE_dartdoc_internal_only\%\%' instead?

@jdkoren jdkoren merged commit d858abc into dart-lang:master Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants