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.

Sign up for GitHub

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

Jump to bottom

build(aio): tighten up code autolinking #20468

Closed
wants to merge 1 commit into from
Closed

build(aio): tighten up code autolinking #20468

wants to merge 1 commit into from

Conversation

petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Nov 15, 2017 •
edited

Do not match code "words" that are part of a hyphenated
string of characters: e.g. platform-browser-dynamic should
not auto-link browser.

Do not match code "words" that correspond to pipe names
but are not preceded by a pipe | character. E.g. package.json should not auto link json to the JsonPipe.

Closes #20187

@petebacondarwin petebacondarwin added comp: aio action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Nov 15, 2017
@petebacondarwin petebacondarwin added this to REVIEW in docs-infra Nov 15, 2017
@mary-poppins
Copy link

You can preview 2b9148d at https://pr20468-2b9148d.ngbuilds.io/.

gkalpak
gkalpak approved these changes Nov 16, 2017
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

A couple of minor comments. LGTM (as soon as CIs are happy - rebasing on master should fix one of the issues) 👍

aio/tools/transforms/post-process-package/processors/post-process-html.js Outdated
@@ -41,7 +41,7 @@ module.exports = function postProcessHtml(log, createDocMessage) {
});
doc.vFile = vFile;
} catch(e) {
throw new Error(createDocMessage(e.message, doc));
throw new Error(createDocMessage(e.stack, doc));
Copy link
Member

Choose a reason for hiding this comment

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

Does this require changes in specs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I guess so. I might drop ditch this change for now. It just made debugging the failures easier.

aio/tools/transforms/angular-base-package/services/filterPipes.spec.js Outdated
});

it('should ignore docs that are pipes, match the pipe name and are preceded by a pipe character', () => {
const docs = [{ docType: 'pipe', name: 'B', pipeOptions: { name: 'b' } }];
Copy link
Member

Choose a reason for hiding this comment

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

In order for the pipe name to match it should be name: "'b'".

aio/tools/transforms/angular-base-package/services/filterPipes.spec.js Outdated

describe('filterPipes', () => {
it('should ignore docs that are not pipes', () => {
const docs = [{ docType: 'class', name: 'B', pipeOptions: { name: 'b' } }];
Copy link
Member

Choose a reason for hiding this comment

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

Might be better to have matching pipe name, to demonstrate that it is the docType that makes the difference.

aio/tools/transforms/angular-base-package/post-processors/auto-link-code.spec.js Outdated
@@ -51,6 +52,26 @@ describe('autoLinkCode post-processor', () => {
expect(doc.renderedContent).toEqual('<code>MyClass</code>');
});

it('should ignore code items match an API doc but are attached to other text via a dash', () => {
Copy link
Member

Choose a reason for hiding this comment

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

match --> that match

aio/tools/transforms/angular-base-package/post-processors/auto-link-code.js
@@ -38,12 +45,13 @@ module.exports = function autoLinkCode(getDocFromAlias) {
parent.children.splice(index, 1, createLinkNode(docs[0], node.value));
} else {
// Parse the text for words that we can convert to links
const nodes = textContent(node).split(/([A-Za-z0-9_]+)/)
const nodes = textContent(node).split(/([A-Za-z0-9_-]+)/)
.filter(word => word.length)
Copy link
Member

Choose a reason for hiding this comment

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

Not directly related to this PR, but filter(word => word.trim()) would avoid processing whitespace only tokens.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this does not work. We need the whitespace tokens because as we rebuild the text after adding in the links. Without these tokens some of the whitespace gets lost.

@gkalpak gkalpak added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 16, 2017
@mary-poppins
Copy link

You can preview 66152fb at https://pr20468-66152fb.ngbuilds.io/.

@petebacondarwin
build(aio): tighten up code autolinking
1491b43 
Do not match code "words" that are part of a hyphenated
string of characters: e.g. `platform-browser-dynamic` should
not auto-link `browser`.

Do not match code "words" that correspond to pipe names
but are not preceded by a pipe `|` character. E.g. `package.json` should
not auto link `json` to the `JsonPipe`.

Closes angular#20187
@mary-poppins
Copy link

You can preview 1491b43 at https://pr20468-1491b43.ngbuilds.io/.

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 16, 2017
@petebacondarwin petebacondarwin moved this from REVIEW to MERGE in docs-infra Nov 16, 2017
@mhevery mhevery closed this in 7c44637 Nov 17, 2017
@petebacondarwin petebacondarwin removed this from MERGE in docs-infra Nov 17, 2017
mhevery pushed a commit that referenced this pull request Nov 18, 2017
@petebacondarwin @mhevery
build(aio): tighten up code autolinking (#20468)
8ddbed8 
Do not match code "words" that are part of a hyphenated
string of characters: e.g. `platform-browser-dynamic` should
not auto-link `browser`.

Do not match code "words" that correspond to pipe names
but are not preceded by a pipe `|` character. E.g. `package.json` should
not auto link `json` to the `JsonPipe`.

Closes #20187

PR Close #20468
@petebacondarwin petebacondarwin deleted the issue-20187-auto-code-links-fixes branch November 22, 2017 17:12
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gkalpak gkalpak gkalpak approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug(aio): incorrect recognization of normal words inside code block
4 participants
@petebacondarwin @mary-poppins @gkalpak @googlebot