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

wip: feat: Make foam respect the .gitignore file #1413

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

TieWay59
Copy link

impl #1388

@TieWay59
Copy link
Author

TieWay59 commented Nov 20, 2024

I have already fixed some issues with Uri.file and should replace it with something like Uri.joinPath for usage. I am not sure if this is a standard practice. @riccardoferretti

The current effect is that all ignored information is read upon program initialization:

image

However, if the ignore is updated, it will not refresh due to saving the file.

image

TODO:

  1. Ensure that the semantics of the .gitignore file are correctly represented in glob ignore patterns.
  2. Design logic so that updates to the ignore file are dynamically read when the command foam: update wikilink def is executed. I want to consult with @riccardoferretti on whether this is reasonable. What code locations should I refer to?

UPD: The .gitignore file is actually an extension of glob patterns, adding more path rules, comments, and negation features. I can easily parse comments, and negation features, and path rules might differ less from glob patterns.

@riccardoferretti
Copy link
Collaborator

Regarding point n.2 you can refer to how we handle changing the related configuration, e.g. foam.files.ignore - see extension.ts:93. Basically, i consider acceptable to ask the user to reload the window to apply the updates

@TieWay59
Copy link
Author

TieWay59 commented Nov 25, 2024

[foam.foam-vscode]: Cannot register 'foam.logging.level'. This property is already registered.

Activating extension 'foam.foam-vscode' failed: command 'foam-vscode.set-log-level' already exists.

@riccardoferretti Hello, I encountered a bug today while testing my modifications. I'd like to ask for some ideas on how to proceed. I've been running code that is essentially identical to this PR, and I'm still experiencing the issue.

2024-11-25 21:26:32 UPD I often encountered this issue during debugging before, and later deduced that it might be caused by the foam-lite plugin. After uninstalling it, the problem seemed to be temporarily resolved.

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.

Left a few comments :)

@@ -201,6 +203,29 @@ export async function createMatcherAndDataStore(excludes: string[]): Promise<{
const excludePatterns = new Map<string, string[]>();
workspace.workspaceFolders.forEach(f => excludePatterns.set(f.name, []));

Logger.info('[wtw] Excluded patterns from settings: ' + excludes);
// Read .gitignore files and add patterns to excludePatterns
for (const folder of workspace.workspaceFolders) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this block of code should live here, I would have it together with the rest of the configuration, in order to populate the excludePatterns variable, which is then fed into this fn.

Copy link
Author

Choose a reason for hiding this comment

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

I'll rework it later, can you hint me more about rest of the configuration ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry what do you mean? My understanding is that your blocker is only the notification to the user not working (and unfortunately I can't understand why that's the case) - am I missing something else?

Copy link
Author

Choose a reason for hiding this comment

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

oh yes, I was so blocked in the dispose problem, so I didn't give it actual implementation and everything else is still in the experiment stage. now I'm about to do it. I'll change a lot design in the next push, so I guess we can ignore this outdated change for now.

@@ -100,6 +100,13 @@ export async function activate(context: ExtensionContext) {
'Foam: Reload the window to use the updated settings'
);
}
}),
// 2024-11-25 21:22:48 TODO wip not working as expected
workspace.createFileSystemWatcher('.gitignore').onDidChange(e => {
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 live inside context.subscriptions so that it's properly cleaned up afterwards

Copy link
Author

Choose a reason for hiding this comment

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

It's true! The issue is that the FileSystemWatcher may be disposed if it's not added to context.subscriptions. problem solved.

Logger.info('[wtw] Excluded patterns from settings: ' + excludes);
// Read .gitignore files and add patterns to excludePatterns
for (const folder of workspace.workspaceFolders) {
const gitignoreUri = Uri.joinPath(folder.uri, '.gitignore');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This path needs to be the same as the file(s) you are watching for changes. (this should be easy to do if you have both at the top level).
I would expect something like this in extension.ts:

  1. compute .gitignore path(s)
  2. extract exclude patterns from those
  3. merge them with the regular configuration settings and use them in this fn
  4. monitor the paths computed

packages/foam-vscode/src/extension.ts Outdated Show resolved Hide resolved
Comment on lines +43 to +66
const gitignoreFiles = await workspace.findFiles('**/.gitignore');

gitignoreFiles.forEach(gitignoreUri => {
try {
workspace.fs.stat(gitignoreUri); // Check if the file exists
const gitignoreContent = await workspace.fs.readFile(gitignoreUri);

// TODO maybe better to use a specific gitignore parser lib.
let ignore_rules = Buffer.from(gitignoreContent)
.toString('utf-8')
.split('\n')
.map(line => line.trim())
.filter(line => !isEmpty(line))
.filter(line => !line.startsWith('#'))
.forEach(line => excludes.push(line));

excludes.push(...ignore_rules);
Logger.info(
`Excluded patterns from ${gitignoreUri.path}: ${ignore_rules}`
);
} catch (error) {
Logger.error(`Error reading .gitignore file: ${error}`);
}
});
Copy link
Author

Choose a reason for hiding this comment

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

@riccardoferretti Do you think here is a reasonable place to calculate ignore rules? I tend to append them into exclude variable.

[info - 6:03:40 PM] [wtw] Starting Foam
[info - 6:03:40 PM] [wtw] Excluded patterns from settings: **/.foam/**,**/.vscode/**/*,**/_layouts/**/*,**/_site/**/*,**/node_modules/**/*,**/.git,**/.svn,**/.hg,**/CVS,**/.DS_Store,**/Thumbs.db,**/.classpath,**/.factorypath,**/.project,**/.settings
[info - 6:03:47 PM] Excluded patterns from /tmp/test_folder/.gitignore: ignored_note.md
[info - 6:03:48 PM] Loading from directories:
[info - 6:03:48 PM] - /tmp/test_folder
[info - 6:03:48 PM]   Include: **/*
[info - 6:03:48 PM]   Exclude: ignored_note.md,**/.foam/**,**/.vscode/**/*,**/_layouts/**/*,**/_site/**/*,**/node_modules/**/*,**/.git,**/.svn,**/.hg,**/CVS,**/.DS_Store,**/Thumbs.db,**/.classpath,**/.factorypath,**/.project,**/.settings
[info - 6:03:49 PM] Workspace loaded in 1071ms
[info - 6:03:49 PM] Graph loaded in 2ms
[info - 6:03:49 PM] Tags loaded in 0ms
[info - 6:03:49 PM] [wtw] Excluded patterns from settings:
[info - 6:03:49 PM] [wtw] Excluded patterns from settings:
[info - 6:03:49 PM] Loaded 2 resources
[info - 6:03:50 PM] Excluded patterns from /tmp/test_folder/.gitignore: ignored_note.md
[info - 6:03:50 PM] Excluded patterns from /tmp/test_folder/.gitignore: ignored_note.md

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.

2 participants