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

Add the possibility of specifying more than one source path ... #301

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ldorigo
Copy link

@ldorigo ldorigo commented Mar 4, 2022

… in which case the destination is assumed to be a folder in which source paths are linked.

This is to solve #301. The diff on github is borked and shows lots of changes, but the changes are actually quite small:

  • Link._default_sources was changed to return a list of sources rather than a single source
  • path is changed to paths and all of the linking action is wrapped in an additional loop that iterates over source paths
  • If globbing is active, additional tests are added for edge cases
  • If there is no globbing and several paths are given, the destination path is assumed to be a directory, so we adapt the destination path accordingly

If you're ok with the approach I'll also edit the documentation. I don't really understand how the tests work, but I'll also try to add a few test cases when I have time to look more into it.

… case the destination is assumed to be a folder in which source paths are linked
@anishathalye
Copy link
Owner

Thanks for the PR! I haven't taken a close look at the diff/code yet, but my high level understanding is that this seems to be useful functionality that is backwards-compatible.

I'll take a closer look in a couple days.

If it's effort to get the tests running locally, don't worry about it, I can add a test based on your example in #300 (comment).

@ldorigo
Copy link
Author

ldorigo commented Jun 4, 2022

Hey, I haven't had time to work more on this and I'm handing in my master's thesis soon so won't have time to look at it for a while more --- if you want to proceed with adding a couple of tests and closing this issue, please don't wait on me :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants