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

Ignore files listed in .mdbookignore during build #1908

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Bergmann89
Copy link

Implementation according to issue #1187 and #1156

@Bergmann89 Bergmann89 force-pushed the master branch 4 times, most recently from 7f6adc7 to 92b89b5 Compare October 13, 2022 14:31
@ehuss
Copy link
Contributor

ehuss commented Oct 13, 2022

Thanks for the PR! Per the comment at #1187 (comment), can this be a setting in book.toml instead?

@Bergmann89
Copy link
Author

I can add this to the config too, but I would like to have this feature as ignore file.

@schungx
Copy link

schungx commented Oct 14, 2022

An ignore file has the benefit of not polluting the main config file and so it can really be done on a user-by-user basis.

The pitfall is... of course... yet another file.

@Bergmann89
Copy link
Author

Just realized, that the gitignore crate has a bug :/ The following patterns will not ignore any file, but it should ignore everything except the mdbook-plantuml-img directory:

/*
!mdbook-plantuml-img

gitignore was not maintained since two years. Maybe it's better to have an own implementation for the ignore files or use the config only. If we add this to the config, it would be nice to have a setting for include and a exclude.

@Bergmann89
Copy link
Author

The ignore crate work as expected.

@Bergmann89
Copy link
Author

I've pushed a new version that uses the ignore crate instead of gitignore.

With the ignore crate we can do some further improvements:

  • Remove the custom implementation of file walk copy_files_except_ext with ignore::Walk which will have the benefit, that also ignore files in the sub directories will be evaluated.
  • Use ignore::Walk to evaluate all .gitignore files (not only in the root directory) to filter files in the watch command.

@Bergmann89 Bergmann89 force-pushed the master branch 3 times, most recently from 74fb51e to f144b57 Compare October 14, 2022 12:12
@ehuss
Copy link
Contributor

ehuss commented Oct 15, 2022

Ok, seems fair to have a separate file. I personally prefer to keep things in one place, and to not have too many files, just to keep things simple (there's one place to describe the config).

I think switching to ignore sounds great.

Can you add some documentation and tests?

@Bergmann89
Copy link
Author

Any remarks? Or can we merge this? :)

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

Overall I'm finding the difference between .gitignore and .mdbookignore a little confusing.

I'm guessing that watch does not ignore .mdbookignore files for a reason? Can you say more about why that choice was made?

@@ -94,13 +95,13 @@ pub fn copy_files_except_ext(
to: &Path,
recursive: bool,
avoid_dir: Option<&PathBuf>,
ext_blacklist: &[&str],
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a public API, the API can't change. I think you will need to create a new function. Also, the naming would no longer be appropriate (_ext was filtering on extensions, and this is now filtering on ignore).

Copy link
Author

Choose a reason for hiding this comment

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

Done. I've added a new public function copy_files_except_ignored that is called by copy_files_except_ext to not have code duplication.

src/book/mod.rs Outdated
@@ -856,4 +857,21 @@ mod tests {
let got = preprocessor_should_run(&BoolPreprocessor(should_be), &html, &cfg);
assert_eq!(got, should_be);
}

#[test]
fn build_test_book() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this test's intent is to validating the mdbookingore behavior, I think it is good to make sure the name reflects that.

Suggested change
fn build_test_book() {
fn mdbookignore_ignores_file() {

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

src/utils/fs.rs Outdated
Comment on lines 132 to 161
if let Some(ignore) = ignore {
let path = entry.path();
if ignore.matched(&path, path.is_dir()).is_ignore() {
continue;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this clause is identical to the one below, can it be moved up out of the if clause and unified?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@@ -279,6 +279,16 @@ The value can be any valid URI the browser should navigate to (e.g. `https://rus
This will generate an HTML page which will automatically redirect to the given location.
Note that the source location does not support `#` anchor redirects.

### `[.mdbookignore]`

You can use a `.mdbookignore` file to exclude files from the build process. The file is places in the `src` directory of your book and has the format known from [`.gitignore`](https://git-scm.com/docs/gitignore) files.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can use a `.mdbookignore` file to exclude files from the build process. The file is places in the `src` directory of your book and has the format known from [`.gitignore`](https://git-scm.com/docs/gitignore) files.
You can use a `.mdbookignore` file to exclude files from the build process. The file is placed in the `src` directory of your book and has the same format as [`.gitignore`](https://git-scm.com/docs/gitignore) files.

Also, can you split it to one sentence per line to make diffs easier to manage?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@@ -279,6 +279,16 @@ The value can be any valid URI the browser should navigate to (e.g. `https://rus
This will generate an HTML page which will automatically redirect to the given location.
Note that the source location does not support `#` anchor redirects.

### `[.mdbookignore]`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is in square brackets. The other sections are like that because they are describing TOML sections of book.toml.

Suggested change
### `[.mdbookignore]`
### `.mdbookignore`

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

@ehuss ehuss added the S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. label Jan 16, 2023
@Bergmann89
Copy link
Author

Overall I'm finding the difference between .gitignore and .mdbookignore a little confusing.

What exactly? We can also add this to the config if you want to, but I need any solution to ignore specific files processed by mdbook.

I'm guessing that watch does not ignore .mdbookignore files for a reason? Can you say more about why that choice was made?

Should not. If this is the case, this is a mistake on my side. I will check this.

I'm currently really busy in my own projects. I will resolve all of your findings once I have some more time for this. This may take a couple of weeks.

Greetings,
Bergmann89.

@ehuss
Copy link
Contributor

ehuss commented Jan 18, 2023

What exactly? We can also add this to the config if you want to, but I need any solution to ignore specific files processed by mdbook.

There is a subtle difference between them (as indicated by the confusion around watch). gitignore files are copied, but not watched. mdbookignore files are not copied but are watched. The documentation doesn't make the differences clear to me, or explain why one would use one over the other. I might suggest in places where gitignore is mentioned in the documentation to make links to the mdbookignore section ("gitignore will only affect which files are watched for changes. To avoid copying certain source files to the output, see [mdbookignore]" or something like that).

I'm also wondering if copying should also avoid git ignored files. However, I think that will require opening the git repo and verifying which files are actually ignored similar to how Cargo itself works, but that is a pretty big hairy mess.

@egasimus
Copy link

Any news of this? Would be happy to help

@Bergmann89
Copy link
Author

Sorry, I had no time to make this ready yet. Feel free to fix the findings above :)

@ehuss ehuss added the A-ignore Area: gitignore, ignoring files label Feb 25, 2024
@Bergmann89 Bergmann89 force-pushed the master branch 3 times, most recently from 79829cd to 3d838ef Compare March 7, 2024 12:31
@Bergmann89
Copy link
Author

@ehuss I (finally) had some time to resolve the findings and rebased to the current master. Might there be some time for another review? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ignore Area: gitignore, ignoring files S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants