-
Notifications
You must be signed in to change notification settings - Fork 644
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 support for wikilinks disambiguation #841
Conversation
b0b3331
to
d3cd8f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor and late (sorry) comments.
function hasExtension(path: string): boolean { | ||
const dotIdx = path.lastIndexOf('.'); | ||
return dotIdx > 0 && path.length - dotIdx <= 4; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test reliable? There are extensions with more than 4 characters. Perhaps checking if the dot index is greater than the last path separator index? Or using path.extname from node (I'm not a node programmer, I don't really know what I'm talking about)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be conflicting criteria for this in other parts of the code.
For example, in utils.ts:
export function dropExtension(path: string): string {
const parts = path.split('.');
parts.pop();
return parts.join('.');
}
This code is strangely repeated in index.ts:
export function dropExtension(path: string): string {
const parts = path.split('.');
parts.pop();
return parts.join('.');
}
which probably requires some cleanup and deduplication.
Maybe you can check that path
equals dropExtension(path)
in order to keep the logic in one (well, two :P) place. But the implementation of dropExtension
isn't bullet-proof either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this method of URI:
static getFileNameWithoutExtension(uri: URI) {
return URI.getBasename(uri).replace(/\.[^.]+$/, '');
}
and also in URI:
static computeRelativeURI(reference: URI, relativeSlug: string): URI {
// if no extension is provided, use the same extension as the source file
const slug =
posix.extname(relativeSlug) !== ''
? relativeSlug
: `${relativeSlug}${posix.extname(reference.path)}`;
posix.extname
seems the safer option to me. There are many different criteria that could be unified around it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these comments @memeplex, all very good observations.
For what regards hasExtension
, yeah, it is an approximation.
Basically you are suggesting simply checking if a .
exists in the last path segment... thinking of it, it's a good approach generally speaking, but I don't think it would work in the context in which hasExtension
is used in workspace
(which makes me think maybe the name of the function is incorrect in the first place).
You will see what I mean if you change the implementation of the function.
For what regards dropExtension
, yes we should dedup them, and consolidate where all these URI/path related utility functions live. Currently I think they should mostly be in URI
, although this one for example is not strictly about the URI
but adjacent.
For what regards posix
, yes, I agree, with a big caveat that I haven't checked yet: I want Foam to soon become available as a web extension, which means there are certain limitations on what OS specific libraries we can use. I don't know whether posix
falls under that. If we can use it in a web extension, then I agree everything should leverage the library.
Does this provide a bit of background? What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most important point is to unify all these criteria in one place, then it will be easy to change it to/from posix or whatever when the time comes. The primitive may be getExtension
, it's easy to implement hasExtension
and dropExtension
from it, maybe they're not even worth of independently existing as small utilities and may be inlined in each usage site. It's unclear to me where something like that should live, the situation with utilities looks a bit messy: utils.ts, utils/, core/utils/. Since there already is dropExtension
in utils.ts that may be the place to implement getExtension
(and, perhaps, also dropExtension
and hasExtension
). Having these extension related functions suggests a path utility class and that already exists in node but given your concerns I would stop at a few simple facade functions. Another option is to make them part of URI but that's already the most bloated and convoluted class in foam, it think it needs to be simplified and not made more complex than it already is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see URI already uses posix quite a lot: extname, dirname, join. I would like to avoid NIH but if you feel that the dependency on posix is worth abstracting in order to be removed later, something like utils.Path or utils.path could make sense. Of course, not a full path utility but just a place where to put path related utilities (object oriented like Path(path).getExtension()
or just functions like utils.path.getExtension(path)
). Again, an alternative is to hide extension manipulation behind URI so everything posix is encapsulated there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, where these functions would live in utils/path.ts
or even in URI.ts
(but I tend to prefer the former).
I need to check whether posix
can be used in a web extension (it might), but in any case I think it would be good to consolidate all path related utility functions (which are an overlapping, but not super nor sub set, of posix
), and put there whatever is required.
In the codebase we use more the functions approach.
I would expect URI
to use these utility functions where necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://code.visualstudio.com/api/extension-guides/web-extensions:
Node.js globals and libraries such as process, os, setImmediate, path, util, url are not available at runtime. They can, however, be added with tools like webpack. The webpack configuration section explains how this is done.
https://code.visualstudio.com/api/extension-guides/web-extensions#webpack-configuration
Not sure what the meaning of something like process or os would be inside such an environment, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you understand how https://code.visualstudio.com/api/extension-guides/web-extensions#webpack-configuration helps with adding those modules?
Because I sure don't 😄
(also, I am not a webpack expert, I'd need to take a deeper look at it, maybe @digitarald can also point us in the right direction)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I haven't the faintest idea. I understand that it bundles the modules but how these modules may be meaningful outside of their natural environment I don't know. Path manipulation is thinkable inside a browser, but most of process, os and other modules mentioned there would be useless.
In this PR the way wikilinks are handled changes in order to support disambiguation across directories.
See the documentation in the PR for more information.
In summary, the resource
/work/projects/foam/refactoring.md
can be identified by[[refactoring]]
,[[foam/refactoring]]
,[[projects/foam/refactoring]]
and[[work/projects/foam/refactoring]]
(and of course via a path-like wikilink).Related: #806
Backwards compatibility