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 resolution of leading-slash page links and add link tests #1108

Merged
merged 2 commits into from
Apr 7, 2021
Merged

Fix resolution of leading-slash page links and add link tests #1108

merged 2 commits into from
Apr 7, 2021

Conversation

brennen
Copy link
Member

@brennen brennen commented Mar 22, 2021

Hopefully this solves #1084, "Page links with leading slash lead to a file in working directory, not a page at the root of the wiki", introduced in 850aace.

It also adds a set of tests for different kinds of wiki links.

TODO:

  • Markdown versions of link tests
    • Do not worry about MediaWiki
  • Clarify documentation about leading // links
  • Could stand to test vimwiki#base#resolve_link() directly (if plausible)
    • Yes, API test is even much easyer. maybe prefix your test with api_

Steps for submitting a pull request:

  • ALL pull requests should be made against the dev branch!
  • Take a look at CONTRIBUTING.MD
  • Reference any related issues.
  • Provide a description of the proposed changes.
  • PRs must pass Vint tests and add new Vader tests as applicable.
  • Make sure to update the documentation in doc/vimwiki.txt if applicable,
    including the Changelog and Contributors sections.

@brennen brennen added the link Related to link and path creation, modification, use [See #478] label Mar 22, 2021
@brennen brennen marked this pull request as draft March 22, 2021 05:02
@tinmarino
Copy link
Member

Nice job. You added two file in testwiki => please update the /testplugin/test/link_generation.vader to expect what it gets (2 new files).
https://travis-ci.com/github/vimwiki/vimwiki/jobs/492673940.
I'll update your TODO in comment with some notes for each point.

@brennen
Copy link
Member Author

brennen commented Mar 23, 2021

Nice job. You added two file in testwiki => please update the /testplugin/test/link_generation.vader to expect what it gets (2 new files).

Ah, thanks. First time I've written anything for vader, so I'm fumbling around a bit. I'll try to finish the pull request tonight, with some tests for Markdown syntax links.

@brennen brennen changed the title WIP: fix resolution of leading-slash page links and add link tests Fix resolution of leading-slash page links and add link tests Mar 25, 2021
@brennen brennen marked this pull request as ready for review March 25, 2021 03:01
@brennen
Copy link
Member Author

brennen commented Mar 25, 2021

@tinmarino any idea why test/config_vars.vader is failing on some versions?

I think other test failures here are already present on dev.

@brennen brennen added this to In progress in v2.6 Mar 25, 2021
@tinmarino
Copy link
Member

@brennen I don't exactly know but the vim/vimwiki session is shared across files so the modified configuration can affect next tests.
Each test must try to be independent. It was hard to test the vimwiki configuration variables, I tried, maybe I did it bad.
In doupt, comment it with a comment why you commnent, I'll see it later.

I prefer all tests to pass with this PR rather than keeping old broken tests

Hopefully this solves #1084, "Page links with leading slash lead to a
file in working directory, not a page at the root of the wiki", introduced
in 850aace.

It also adds a set of tests for different kinds of wiki links:

  - api_base_resolve_link.vader
  - link_syntax_markdown.vader
  - link_syntax_vimwiki.vader

Includes some fixes for locally-failing tests, removes a test from
test/map.vader and comments out test/config_vars.vader entirely for the
moment.
@brennen
Copy link
Member Author

brennen commented Mar 30, 2021

I prefer all tests to pass with this PR rather than keeping old broken tests

I wound up commenting out 2: the config variable tests, and the one for autocompletion in a list in map.vader.

I also disabled the GetHighlightTerm() invocations for versions less than 8.0, since execute() isn't reliably available in 7.4 prior to patch 7.4.2008.

Copy link
Member

@tinmarino tinmarino left a comment

Choose a reason for hiding this comment

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

Good,
Most of the changes are in tests, comments and better variable names.
The tests are passing so this time I push the green button.

PS: I prefer no to squash your commits as trying to minimise the commit number is a bad pattern.
Edit: plus it is only 2 commit (I thought 17 ...)
Edit: I had to squash it anyway.

if is_wiki_link && link_text[0] ==# '/'
if link_text !=# '/'
if link_text !=# '//' && link_text[0:1] ==# '//'
let link_text = resolve(expand(link_text))
let link_text = link_text[2:]
let is_absolute = 1
let is_absolute_wiki_link = 1
Copy link
Member

Choose a reason for hiding this comment

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

Yes !
Good: explicit better than implicit especially when talky about absoluteness

complete3
* complete2

# brennen commenting this out 2021-03-29 - test seems to flap, test failures
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I did not mention it but this file gave many troubles.

Ranebrown did isolate it in a custom docker session. I tryed to play with the order to go faster but it may be guilty from bugs 20 tests later.

if v:version < 704 | return [] | endif
"
" Warning: must only be called if has("patch-7.4-2008")
" Or rather: If execute() exists - it's not available for all 7.4
Copy link
Member

Choose a reason for hiding this comment

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

This makes it more clear (and mantainable)

@@ -3894,6 +3898,7 @@ Contributors and their Github usernames in roughly chronological order:
- Yifan Hu (@yhu266)
- Levi Rizki Saputra (@levirs565)
- Fergus Collins (@C-Fergus)
- Brennen Bearnes
Copy link
Member

Choose a reason for hiding this comment

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

Lol ! I put mine on the first commits.

@tinmarino tinmarino merged commit f396e8a into vimwiki:dev Apr 7, 2021
@brennen brennen deleted the link-resolution branch April 27, 2021 18:27
@brennen brennen moved this from In progress to Done in v2.6 Apr 27, 2021
jls83 pushed a commit to jls83/vimwiki that referenced this pull request Jan 17, 2023
Hopefully this solves vimwiki#1084, "Page links with leading slash lead to a
file in working directory, not a page at the root of the wiki", introduced
in 850aace.

It also adds a set of tests for different kinds of wiki links:

  - api_base_resolve_link.vader
  - link_syntax_markdown.vader
  - link_syntax_vimwiki.vader

Includes some fixes for locally-failing tests, removes a test from
test/map.vader and comments out test/config_vars.vader entirely for the
moment.

Code by : Brennen Bearnes <code@p1k3.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
link Related to link and path creation, modification, use [See #478]
Projects
No open projects
v2.6
  
Done
Development

Successfully merging this pull request may close these issues.

Page links with leading slash lead to a file in working directory, not a page at the root of the wiki
2 participants