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 #2183

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

Conversation

egasimus
Copy link

@egasimus egasimus commented Sep 3, 2023

Based on #1908 by @Bergmann89.

This fixes the issues identified by @ehuss, except for the updated copy_files_except_ext function:

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).

I would like to discuss that.

the naming would no longer be appropriate

Agreed, I'd call it just copy_files

I think you will need to create a new function.

In which case copy_files_except_ext would be orphaned within the mdBook codebase - as far as I saw, it has a single caller.

Maybe some package that depends on mdbook uses this function, maybe none does; information flows the other way, so we have no way of knowing, and no way of addressing that, except, of course, for... semver (which Cargo helpfully enforces).

Since this is a public API, the API can't change

As far as I understand the semver specification, public API can change as long as the change is denoted by incrementing the major version - except in the case of 0.x versions, where anything goes. Generally, I'm a detractor of using 0.x versions for prolonged periods of time, exactly for this reason.

Maybe, after adding much-awaited support for .mdbookignore, it would be a good time to release a 1.0.0, and start harnessing the full benefits of the semver standard by tracking breaking changes in the major version? That's what it's for 🤷

@rustbot rustbot added the S-waiting-on-review Status: waiting on a review label Sep 3, 2023
@ehuss
Copy link
Contributor

ehuss commented Nov 29, 2023

Regarding the semver compatibility: Yea, the API can change across semver-incompatible releases, but we have to minimize the semver-incompatible releases because they affect plugins (plugins may need to be updated in order to use the new release). We will eventually plan to release a 0.5 release, which will likely include a variety of API changes, but it will likely be a while before that is ready. In order to avoid waiting for that, you can add new functions and avoid changing existing ones. Any stale functions can be removed in the next semver-breaking release (0.5).

@Bergmann89
Copy link

Hey @egasimus, this problem came up to me again. I've pulled your changes and rebased on the latest master. I've also made you a contributor on my fork, so that we are able to work on the same PR.

@pnkfelix
Copy link
Member

Hey @egasimus and @Bergmann89 , it sounds to me like this PR could get over the finish line if it is "just" changed to avoid the API-breaking change to copy_files_except_ext, by adding a new function.

I'm willing to look into make that additional change here, but I figured I'd ask if either of you two want to take that on first, rather than stepping on your toes.

@Bergmann89
Copy link

Hey @pnkfelix,

currently I have no time to work on this, but I've added you as a collaborator to my fork. Feel free to finish the work.

Greetings, Bergmann.

@egasimus
Copy link
Author

Same here! I can't make the context switch into this right now (dammit >_<), but by all means do proceed @pnkfelix, it would be awesome for anyone to finalize this! 🚀

@Razer6
Copy link

Razer6 commented Jul 22, 2024

Whats the state of this PR? I sitting in front of the same problem. I am happy to take a look and bring it to the finish line if needed.

@Bergmann89
Copy link

Hey @Razer6,

I've added the changes from @egasimus to the original PR (#1908) a while ago, but unfortunately I din't had time to finish it. I added you as a collaborator to my fork. Feel free to finish the PR.

Greetings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: waiting on a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants