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

join files recipe - remove './' from example, fixes #457 #460

Merged
merged 2 commits into from
May 12, 2022

Conversation

kieranjol
Copy link
Collaborator

@kieranjol kieranjol commented May 10, 2022

This hopefully fixes #457 .
I added a note as well that the file path should not be the absolute path in the textfile. I think that this might be less obvious now that the ./ has been removed in the example.
Edit: I didn't use the term 'relative path' as I was worried that this might lead people to want to use the './'

@kfrn
Copy link
Member

kfrn commented May 10, 2022

Cheers @kieranjol! That LGTM as a quick update.

Any chance you'd want to expand at all? @brainwane put it perfectly:

So, it sounds like the recipe should advise people: if possible, run the command from the same directory where the files and the txt file reside. Otherwise you'll have to use safe (and here's how).

Otherwise, I could look at this later on.

@privatezero
Copy link
Member

Looks good to me, although I agree that the little additional clarification suggested by @brainwane and @kfrn - I find that whenever I use concat that I tend to do exactly what is mentioned there so seems kind to users to include that tidbit!

@kieranjol
Copy link
Collaborator Author

Desperately hoping that I did this correctly, it's been a while since I made commits within github itself! I totally agree that it should have been added in in the first place!

@kieranjol
Copy link
Collaborator Author

Also that intro seemed like the best place to put it?

@kfrn
Copy link
Member

kfrn commented May 10, 2022

LGTM @kieranjol!

@kieranjol
Copy link
Collaborator Author

Can this be merged?

@kieranjol kieranjol merged commit 6303394 into amiaopensource:gh-pages May 12, 2022
@kieranjol
Copy link
Collaborator Author

Just remembered that I can merge it after the approvals! Thx folks.

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 this pull request may close these issues.

Joining files together, "safe", and relative filenames
5 participants