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

Add functionality to link arbitrary strings in code blocks #1576

Merged
merged 7 commits into from
Jul 15, 2020

Conversation

rogermparent
Copy link
Contributor

@rogermparent rogermparent commented Jul 14, 2020

currently the only usage is dvc.yaml, but this addition to the remark plugin is able to handle any matchers, simple to complex. If we find the need for generating urls based on content I can add it, but since it isn't necessary for the currently requested additional links it can be a low priority concern.

Fixes #1431

currently the only usage is `dvc.yaml`, but this addition should be able to
support any auto-linking rules we want in the future, even complex ones.
@shcheklein shcheklein temporarily deployed to dvc-landing-new-auto-li-fwr1dg July 14, 2020 00:48 Inactive
@rogermparent rogermparent temporarily deployed to dvc-landing-new-auto-li-fwr1dg July 14, 2020 01:15 Inactive
@rogermparent
Copy link
Contributor Author

An example of this working can be found here, (or rather, the first instance of "dvc.yaml" after that heading)

@shcheklein
Copy link
Member

let's run some script to remove already existing links?

@jorgeorpinel could you please provide a list of things to link?

(@rogermparent @jorgeorpinel it can be sone as a separate ticket, feel free to merge this one PR then)

@rogermparent
Copy link
Contributor Author

let's run some script to remove already existing links?

I can ripgrep for [dvc.yaml] and remove all existing instances, replacing them with this behavior.

@shcheklein
Copy link
Member

@rogermparent sounds good to me, let's start with that

This commit provided to you by ripgrep and emacs
@rogermparent rogermparent temporarily deployed to dvc-landing-new-auto-li-fwr1dg July 14, 2020 19:42 Inactive
@rogermparent
Copy link
Contributor Author

Just pushed a commit that "unwraps" all links that contain dvc.yaml, including those that contain other text like "dvc.yaml file", making usage of the auto-link feature for this term consistent across the repo.

@rogermparent
Copy link
Contributor Author

rogermparent commented Jul 14, 2020

(Originally from #1431)

From what I understand, there's three currently proposed entries for the automatic linker

  • dvc.yaml could be like a command name, which automatically links to its user guide
  • "orphan stage" we could link manually to dvc add
  • "import stage" to dvc import

@shcheklein
Copy link
Member

@jorgeorpinel could you please review and share the list related to the DVC files?

@rogermparent
Copy link
Contributor Author

rogermparent commented Jul 14, 2020

It seems like "import stage" and "orphan stage" aren't used in code blocks like the current linker checks for, but I could add additional functionality to operate on italic and bold text in the same way if we want these terms auto-linked but not in code quotes.

...actually, I don't think I check for node type in simpleLinker which may mean this is actually the current behavior. Seems like italic/bold is treated different from code quotes, but I can adapt the code to it if we want this feature.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Jul 15, 2020

Sorry, just noticed this.

@jorgeorpinel could you please provide a list of things to link?

  • .dvc file(s) to /doc/user-guide/dvc-files-and-directories#dvc-files
  • dvc.yaml to /doc/user-guide/dvc-files-and-directories#dvcyaml-file
  • dvc.lock to /doc/user-guide/dvc-files-and-directories#dvclock-file

Or maybe all to just /doc/user-guide/dvc-files-and-directories ?

Don't worry about "orphan/import stgae" — we're not using those terms anymore.

let's run some script to remove already existing links?
I can ripgrep for [dvc.yaml] and remove all existing instances

Yes, this is needed, but for all [`dvc.yaml`], [`dvc.lock`], and `.dvc` file/ `.dvc` files. (We could do this separately if to keep this PR smallish.)

@rogermparent
Copy link
Contributor Author

rogermparent commented Jul 15, 2020

Sorry, just noticed this.

No problem!

* .dvc file(s) to /doc/user-guide/dvc-files-and-directories#dvc-files

* dvc.yaml to /doc/user-guide/dvc-files-and-directories#dvcyaml-file

* dvc.lock to /doc/user-guide/dvc-files-and-directories#dvclock-file

I can definitely do these!

Or maybe all to just /doc/user-guide/dvc-files-and-directories ?

That's up to preference, no difference either way.

Don't worry about "orphan/import stgae" — we're not using those terms anymore.

Sure!

Yes, this is needed, but for all [`dvc.yaml`], [`dvc.lock`], and `.dvc` file/`.dvc` files. (We could do this separately if to keep this PR smallish.)

Considering I've already done it for dvc.yaml, I think it's fine to do this with the rest to introduce the feature.

The last one probably won't work like this though. It's tricky because the back quotes don't wrap the full text...

Correct, currently the auto-linker specifically checks individual MDAST nodes. This works for us because code blocks are always their own individual nodes, even when inline. To capture words around the block, like file in ".dvc file", would require a large amount of re-rigging of the autolinker to consider surrounding words. Autolinking .dvc and unwrapping existing links containing the text, like I did with dvc.yaml, will successfully convert all instances to auto-linked ones.

@jorgeorpinel
Copy link
Contributor

To capture words around the block, like file in ".dvc file", would require a large amount of re-rigging of the autolinker

I say we just do .dvc then. I already changed all the ones referring to the directory to .dvc/ anyway, I think.

BTW maybe we could also add .dvc/ -> https://dvc.org/doc/user-guide/dvc-files-and-directories#internal-directories-and-files — or even //.dvc/[^/]// if regexps are allowed (to catch things like .dvc/cache and .dvc/config). However the scope would further expand so def. should extract that part to a separate issue/PR

@rogermparent rogermparent temporarily deployed to dvc-landing-new-auto-li-fwr1dg July 15, 2020 18:23 Inactive
@rogermparent rogermparent temporarily deployed to dvc-landing-new-auto-li-fwr1dg July 15, 2020 18:33 Inactive
@rogermparent
Copy link
Contributor Author

rogermparent commented Jul 15, 2020

Here's what we've got in this PR

new auto-linked terms:

  • .dvc to /doc/user-guide/dvc-files-and-directories#dvc-files
  • dvc.yaml to /doc/user-guide/dvc-files-and-directories#dvcyaml-file
  • dvc.lock to /doc/user-guide/dvc-files-and-directories#dvclock-file

also

  • existing links containing the previous three terms have been "unwrapped", effectively switching to the auto behavior.

terms must be surrounded in code quotes (backticks) to be recognized by the auto-linker. Terms with parts that aren't code-quoted (like ".dvc file") will only have the code-quoted part linked by the auto-linker. This issue can be fixed in a few ways, but is more of a "nice-to-have" when compared to the rest of this whole feature which is intended to make the editing process easier.

@rogermparent
Copy link
Contributor Author

rogermparent commented Jul 15, 2020

Unless there's an unexpected regression or we think of something else to add, I think we're ready to merge this PR!

@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-new-auto-li-fwr1dg July 15, 2020 18:52 Inactive
to the data source (as explained in the description above). (This import stage
can later be used to [update](/doc/command-reference/update) the import.) Check
`data.xml.dvc`:
but it also creates an import stage (`.dvc` file) with a link to the data source
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for self: replace all these "import stage" term instances in a separate PR.

@shcheklein shcheklein merged commit f3e0f29 into master Jul 15, 2020
@shcheklein
Copy link
Member

thanks @rogermparent and @jorgeorpinel !

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Yeah I was just waiting for https://dvc-landing-new-auto-li-fwr1dg.herokuapp.com/ to deploy for a sanity check, and it looks good.

I also actually reviewed every change and left a couple minimal suggestions, but approving this now!

Comment on lines +3 to +4
When you add a file or a stage to your pipeline, DVC creates a special `.dvc` or
`dvc.yaml` file (respectively) that contains all the needed information to track
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When you add a file or a stage to your pipeline, DVC creates a special `.dvc` or
`dvc.yaml` file (respectively) that contains all the needed information to track
When you add a stage or a file to your pipeline, DVC creates a special `dvc.yaml`
or `.dvc` file (respectively) that contains all the needed information to track

@@ -0,0 +1,14 @@
module.exports = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be extracted to a simple JSON or YAML config file so we can easily edit the list later? I mean... It's already easy but would be even easier 🙂

Also, should a test be written for this fn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could, but using .js allows us to use RegExp literal matchers and dynamic replacement values in a future. It's possible to switch to JSON by simply converting what's there now if we decide we don't want those features.

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.

basic concepts: pipeline stages
3 participants