Fix: Exclude not working with absolute path in some cases (#593) by dqkqd · Pull Request #618 · Kampfkarren/selene

dqkqd · 2024-10-12T06:36:48Z

Exclude sometimes doesn't work with absolute path because it tries to match the pattern directly with the path itself.

Since the exclude option is relative, the absolute path to lint should be converted to relative path as well.

exclude = ["**/file.lua"]
exclude = ["src/**"]
exclude = ["external/*", "*.spec.lua"]

For example, if we are in selene-test folder with the following exclude option:

exclude = ["src/**"]

Then, by converting this absolute path /path/to/selene-test/src/file.lua to src/file.lua, exclude can work properly.

The problem is which path we should relate to, initially I thought using config directory (where selene.toml lies) is a good place, but to keep it simple without unnecessary changes, I use the current directory (where selene is invoked) instead.

chriscerie

Thanks! Need changelogs for:

the fix
changing undocumented behavior that exclude paths are now relative to the config file instead of where selene is invoked

We should also properly document that in https://kampfkarren.github.io/selene/usage/configuration.html#excluding-files-from-being-linted

chriscerie · 2024-11-16T17:49:06Z

selene/src/main.rs

-                        Path::new(&config_file).parent().map(Path::to_path_buf),
-                    ),
+                    Ok(config) => {
+                        // Get config directory to using absolute path.


Suggested change

// Get config directory to using absolute path.

chriscerie · 2024-11-16T17:49:18Z

selene/src/main.rs

+                        // There is case where `config = Some("selene.toml")`,
+                        // which returns `""`` when getting parent directory.


What does this mean?

chriscerie · 2024-11-16T17:49:44Z

selene/src/main.rs

+                        let config_directory = Path::new(&config_file)
+                            .canonicalize()
+                            .ok()
+                            .and_then(|path| path.parent().map(|path| path.to_path_buf()));


chriscerie · 2024-11-16T17:50:02Z

selene/src/main.rs


    let (config, config_directory): (CheckerConfig<toml::value::Value>, Option<PathBuf>) =
        match options.config {
+            // User provides a config file. We must read it and report if it is misconfigured.


Suggested change

// User provides a config file. We must read it and report if it is misconfigured.

chriscerie · 2024-11-16T17:51:05Z

selene/src/main.rs

+    // Directory for matching files pattern.
+    // If user provides a config file, we use that as our working directory, otherwise fallback to the current directory.


Suggested change

// Directory for matching files pattern.

// If user provides a config file, we use that as our working directory, otherwise fallback to the current directory.

chriscerie · 2024-11-16T17:52:12Z

selene/src/main.rs


+    // Directory for matching files pattern.
+    // If user provides a config file, we use that as our working directory, otherwise fallback to the current directory.
+    // The working directory needs to be an absolute path to match with paths in different directories.


This doesn't explain why relative path won't work. Can we either clarify that?

chriscerie · 2024-11-16T18:04:33Z

selene/src/main.rs

+            return false;
+        }
+
+        // File pattern between the path we are checking and current working directory.


Suggested change

// File pattern between the path we are checking and current working directory.

barrettruth · 2025-09-11T23:43:18Z

@dqkqd update on this?

dqkqd · 2025-09-11T23:53:11Z

@barrett-ruth
Oops, I forgot about this, let me see if I can work on this on this weekend.

dqkqd · 2025-09-12T23:57:21Z

@chriscerie
Pardon for my rudeness on the first draft, I added a bunch of unnecessary comments and did not present my solution properly.

changing undocumented behavior that exclude paths are now relative to the config file instead of where selene is invoked

I thought that pattern matching on exclude paths should be relative to the config directory because:

This allows users to invoke selene from anywhere (e.g. from the sub directory) while keep returning consistent result.
It also allows other program (e.g. neovim plugins, vscode) to invoke selene from different folders as well.

However, this might have some edge cases:

Users invoking selene directly might be confuse how exclude works. Though this is rare because most of the time people would run selene in the workspace directory.
How to handle cases where selene.toml file isn't found in the workspace? Should we fallback to where we invoke selene?

What do you think?
Should I keep implementing the solution using relative path on config directory, or just using the folder where we invoke selene for simplicity.

chriscerie · 2025-09-13T16:29:04Z

We should keep existing behavior if changing is not required to fix the issue. Changing it should have its own issue and discussion.

dqkqd · 2025-09-13T21:41:09Z

Thank @chriscerie

We should keep existing behavior if changing is not required to fix the issue. Changing it should have its own issue and discussion.

Then, I use the current directory (where selene is invoked) to convert the absolute path to relative path.

I updated the PR and the PR description as well. Please have a look.

dqkqd force-pushed the issue-593 branch 5 times, most recently from 4293812 to f606559 Compare October 13, 2024 21:54

ccuser44 approved these changes Oct 29, 2024

View reviewed changes

chriscerie reviewed Nov 16, 2024

View reviewed changes

chriscerie marked this pull request as draft July 13, 2025 03:30

dqkqd force-pushed the issue-593 branch 3 times, most recently from e7500af to 2dc6b60 Compare September 12, 2025 23:25

fix: working with absolute filepath (Kampfkarren#593)

cea8f27

dqkqd force-pushed the issue-593 branch from 2dc6b60 to cea8f27 Compare September 13, 2025 21:17

dqkqd marked this pull request as ready for review September 13, 2025 21:41

dqkqd requested a review from chriscerie September 13, 2025 21:42

		// There is case where `config = Some("selene.toml")`,
		// which returns `""`` when getting parent directory.

	.and_then(\|path\| path.parent().map(\|path\| path.to_path_buf()));
	.and_then(\|path\| path.parent().map(Path::to_path_buf));

		// Directory for matching files pattern.
		// If user provides a config file, we use that as our working directory, otherwise fallback to the current directory.

Uh oh!

Conversation

dqkqd commented Oct 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chriscerie left a comment

Choose a reason for hiding this comment

Uh oh!

chriscerie Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

chriscerie Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

chriscerie Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

chriscerie Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

chriscerie Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

chriscerie Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

chriscerie Nov 16, 2024

Choose a reason for hiding this comment

Uh oh!

barrettruth commented Sep 11, 2025

Uh oh!

dqkqd commented Sep 11, 2025

Uh oh!

dqkqd commented Sep 12, 2025

Uh oh!

chriscerie commented Sep 13, 2025

Uh oh!

dqkqd commented Sep 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dqkqd commented Oct 12, 2024 •

edited

Loading

dqkqd commented Sep 13, 2025 •

edited

Loading