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

Cannot copy one file to multiple destinations #428

Open
not-my-profile opened this issue Jun 5, 2023 · 3 comments · May be fixed by #429
Open

Cannot copy one file to multiple destinations #428

not-my-profile opened this issue Jun 5, 2023 · 3 comments · May be fixed by #429

Comments

@not-my-profile
Copy link
Contributor

The Site.copy API is currently quite the footgun:

  • You cannot copy one file to multiple locations.

    site.copy("foo", "bar1")
    site.copy("foo", "bar2")
    

    foo is only copied to bar2.

  • You cannot copy a directory and one file in the directory elsewhere.

    site.copy("static")
    site.copy("static/foo", "bar")
    

    static/foo is not copied to bar.

I would consider both of these to be bugs.

not-my-profile added a commit to not-my-profile/lume that referenced this issue Jun 5, 2023
Site.copy previously didn't support multiple destinations:

   site.copy("foo", "bar1")
   site.copy("foo", "bar2")

Previously resulted in `foo` being only copied once to `bar2`.

   site.copy("static")
   site.copy("static/foo", "bar")

Previously didn't copy `static/foo` to `bar`.

This commit fixes both of these bugs. Fixes lumeland#428.
@not-my-profile not-my-profile linked a pull request Jun 5, 2023 that will close this issue
not-my-profile added a commit to not-my-profile/lume that referenced this issue Jun 5, 2023
Site.copy previously didn't support multiple destinations:

   site.copy("foo", "bar1")
   site.copy("foo", "bar2")

Previously resulted in `foo` being only copied once to `bar2`.

   site.copy("static")
   site.copy("static/foo", "bar")

Previously didn't copy `static/foo` to `bar`.

This commit fixes both of these bugs. Fixes lumeland#428.
@oscarotero
Copy link
Member

Ok, I just saw your pull request (many thanks!) but I'd like to continue the conversation here about how is supposed to work and leave the PR for implementation details.

Is there any real use case in which you want to duplicate a file and save it in multiple destinations?

The most common use case is:

  • I want to copy all files in the directory/statics to the destination root: site.copy("statics", ".").
  • But the files in statics/favicons should be in the root, so I copy them with site.copy("statics/favicons", ".").

What you're proposing is the destination directory had the favicons in two different places: / and /favicons (one per each copy). I think the correct behavior is to have only once, because one copy overrides the other.

Lume doesn't handle this well, because once the static folder is found, it copies all its content to the destination folder without checking other cases inside this folder (like the favicons folder exception).

It's perfectly valid that the same file matches different copy() conditions. But this shouldn't output multiple copies of the same file, it should choose the most specific condition.

For example, I want to copy all .jpg files:

site.copy([".jpg"]);

But I want to copy a specific jpg file to a specific destination:

site.copy("/cover/image.jpg", "/img/cover/image.jpg");

The second condition is more specific than the first, so the image should be copied once to img/cover/image.jpg

To me, the priority is the following (ordered from less specific to most specific):

  • Copy by extension is the less specific (site.copy([".jpg"]))
  • Copy by folder or filename is more specific (site.copy("/img/cover/image.jpg")).
    • Subfolders are more specific than parent folders. /img/cover is more specific than /img and /img/cover/image.jpg is more specific than /img/cover.

@not-my-profile
Copy link
Contributor Author

Is there any real use case in which you want to duplicate a file and save it in multiple destinations?

You could use Lume to generate multiple sites.

I agree that the "copy by extension" mechanism should have a lower priority than the "copy a specific path" mechanism. However I seriously don't think that we should do anything fancy about the "copy a specific path" mechanism because it breaks user expectations.

When it comes to an API the code should do what the code says literally. If a method is called copy, calling it should always attempt to copy. By that logic I'd also rename copyRemainingFiles to e.g. setImplicitCopyDecider and deprecate copy(from: string[], ...) in favor of something like:

site.setImplicitCopyDecider(
  (path: string, ext: string) => [".jpg", ".gif", ".png"].includes(ext),
);

This also makes it much easier to explain the behavior to the user: With the copy function you can perform an explicit copy operation. Files that aren't explicitly copied can still be copied by defining an implicit copy decider function.

@oscarotero
Copy link
Member

You could use Lume to generate multiple sites.

I don't get this. You cannot build multiple sites from a single deno task build. But you want to output different deno task build to the same folder with the emptyDest option set to false.

copyRemainingFiles is not the same as copy(from: string[]). Copying has priority over loading. For example, Lume loads by default all .md files (to parse and generate html pages). If you set copy([".md"]), .md files won't be loaded and only will be copied because Lume checks if a file must be copied before loading it. copyRemainingFiles is used only if the file is not copied nor loaded.

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 a pull request may close this issue.

2 participants