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 file ignore setting (issue #300) #304

Merged
merged 4 commits into from
Nov 3, 2020
Merged

Add file ignore setting (issue #300) #304

merged 4 commits into from
Nov 3, 2020

Conversation

jmg-duarte
Copy link
Member

@jmg-duarte jmg-duarte commented Oct 28, 2020

This PR is related to issue #300, the issue will be closed and discussion moved here.

This serves as a basic implementation for an ignore mechanism for Foam related files, this is useful to avoid files appearing in the graph (see below).

Before:
image

After:
image

Standing problems:

  • Maybe this should be glob based, to be discussed.

  • Settings do not refresh, this also happens for other Foam things, such as the graph.

@jmg-duarte jmg-duarte changed the title Add file ignore setting (#300) Add file ignore setting (issue #300) Oct 28, 2020
@riccardoferretti
Copy link
Collaborator

riccardoferretti commented Oct 28, 2020

thanks @jmg-duarte, this is looking good. The only question I have is why not using globs?
It sounds to me like a solid/flexible/standard solution, what's your thinking around this?

@jmg-duarte
Copy link
Member Author

The glob library goes hand in hand with the file system and is not usable for matching along lists.

@riccardoferretti
Copy link
Collaborator

I am missing something, why is that a problem?
I also noticed the workspace.findFiles function also supports globs with explicit exclude support, would that be a viable option?

@jmg-duarte
Copy link
Member Author

We could use the globs if we put both the list of files found and the ignored ones in sets and intersected them.
Otherwise we would could be comparing a lot of files vs a lot of more files.

I also noticed the workspace.findFiles function also supports globs with explicit exclude support, would that be a viable option?

Probably, I'll do some research on it

@jmg-duarte
Copy link
Member Author

It seems to me that the workplace.findFiles function does not work for this use case due to the limitation of supporting a single exclude.
However I'm curious about the set intersection approach and plan to try and profile both.

@riccardoferretti
Copy link
Collaborator

hi @jmg-duarte, did you get a chance to look into this? No rush, was thinking of shipping it with 0.5.0 (in one week), but wanted to check your status, thanks!

@jmg-duarte
Copy link
Member Author

I got sidetracked due to the lack of big repositories to profile with.
However I believe the set intersection approach is better (at least in big repositories) since comparison is hash-based and not string-prefix.
I can implement the set based approach to be merged and if necessary in the future we switch.

@jmg-duarte
Copy link
Member Author

I tried to use a set but the Uri objects do not "equal" reliably Javascript 🤷, to cope with it I replaced the Set with a Map, comparing on keys, which in this case is the fsPath.
I didn't test performance wise, but I'd say this approach is probably faster.
And even if it is not, it is MUCH cleaner and easier to read/understand.

Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@riccardoferretti riccardoferretti left a comment

Choose a reason for hiding this comment

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

Still looks good to me :) but let's update the description of the property so that it's clearer for users

"array"
],
"default": [],
"description": "List of files/folders Foam should ignore. Paths are relative to the workspace root."
Copy link
Collaborator

Choose a reason for hiding this comment

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

this needs to be updated right? Might also be worth adding an example in parenthesis for clarity

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of an example we can set a default .vscode/* and _layouts/*for example.
Description wise I'll try to make it clearer since the description is out of date

@riccardoferretti riccardoferretti merged commit c10b73c into foambubble:master Nov 3, 2020
@jmg-duarte jmg-duarte mentioned this pull request Nov 11, 2020
5 tasks
@patchworquill
Copy link

Hi all -- having trouble with this. My foam.files.ignore entry doesn't seem to be blocking the globbed folders from appearing in my Foam Graph.

@jmg-duarte
Copy link
Member Author

Hi all -- having trouble with this. My foam.files.ignore entry doesn't seem to be blocking the globbed folders from appearing in my Foam Graph.

Can you show your entry?

@patchworquill
Copy link

Fixed. Didn't realize I had to git commit for the settings to take effect.

I did the following:

  • removed all the extras within _layout (probably not necessary but it felt like clutter since I'm not yet publishing online)
  • added "/.vscode" and "/_layouts" to the settings.json file within the /.vscode folder in my repo (not the global settings file, although likely that would work fine as well)
  "foam.files.ignore": [
    "/.vscode",
    "/_layouts",
  ]
  • added and commited these changes in git

Now my graph is once again clean and useable. :)

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.

3 participants