Skip to content

Conversation

redthor
Copy link
Contributor

@redthor redthor commented Mar 23, 2018

Hi there,

This PR is just to show you what I did. I don't expect it to be merged. I can submit another if you think some of it is worthwhile.

Basically I wanted to optionally pass a folder name with some depth, e.g.

echo '/home/user/repo.git library-package lib/packages/library-package' | tomono.sh

So in the monorepo you'll find ./lib/packages/library-package/*

I think I received an ls error at some point but otherwise it seems to work.
Thanks for the script.

Copy link
Owner

@hraban hraban left a comment

Choose a reason for hiding this comment

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

Hi,

Thanks for your contribution. I think it could be worth merging.

I like the last commit, but the first two introduce a lot of unnecessary noise. Could you remove those and rebase?

And if you could, please add a little comment to the README explaining the new syntax?

Thanks again.

tomono.sh Outdated
@@ -99,6 +99,10 @@ function create-mono {
return 1
fi

if [[ -z "$folder" ]]; then
folder=$name
Copy link
Owner

Choose a reason for hiding this comment

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

to be safe, I'd prefer to always quote any variable in bash. folder="$name"

@redthor
Copy link
Contributor Author

redthor commented Apr 9, 2018

Hi there,
I've made the changes. I wasn't sure about the instruction for rebasing? I think the branch was already up to date with master if that's what you meant?

I've reverted the whitespace commits. I imagine you can squash the PR to remove all the extra commits...

I've made the README change and "$name" change.

Thanks.

@hraban hraban mentioned this pull request Apr 9, 2018
@hraban
Copy link
Owner

hraban commented Apr 9, 2018

Thanks. I've rebased your patches to avoid thrashing the history and paralysing git blame, but otherwise no changes.

To maintainers:

Please don't merge this PR!

Merge #25 instead.

@redthor
Copy link
Contributor Author

redthor commented Apr 9, 2018

@hraban - quick git question: would squashing the merge commit avoid thrashing the history and paralysing git blame?

@hraban
Copy link
Owner

hraban commented Apr 9, 2018

Yes, that's what I did in #25. Squash all commits except the readme, and move the last commit above the readme to squash it in, as well.

Assuming you're on branch feature/folder-name and your master is in line with ours; use git rebase -i master for that and move and mark lines appropriately. You'll want the VISUAL envvar set to your editor, e.g. 'vim' or 'emacs'.

@redthor
Copy link
Contributor Author

redthor commented Apr 10, 2018

Oh - I mean just use github to do it for you? e.g.

see this image

@hraban
Copy link
Owner

hraban commented Apr 10, 2018 via email

@redthor
Copy link
Contributor Author

redthor commented Apr 10, 2018

lol, ok @hraban :)

@sweeney sweeney closed this Apr 11, 2018
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.

3 participants