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

Adds CLI support for partials with relative paths #558

Closed
wants to merge 3 commits into from
Closed

Adds CLI support for partials with relative paths #558

wants to merge 3 commits into from

Conversation

kookookchoozeus
Copy link
Contributor

Currently, if two partials with the same filename are passed through the CLI:

bash
mustache -p src/templates/header.mustache -p src/templates/common/header.mustache view.json template.mustache


only one of them is used (whichever's fs.createReadStream returns last) for the partial

{{> header }}


This change allows you to specify partials via paths relative to the template file

{{ ../common/header }}

@phillipj
Copy link
Collaborator

This sounds like a good fix 👍 Would you be able to add tests verifying this behavior?

@kookookchoozeus
Copy link
Contributor Author

I will be writing tests for this, but I was getting 11 failed tests on windows (before I changed anything), so I figured I would take a look at that while I'm at it, most likely this weekend

@phillipj
Copy link
Collaborator

Ooh, that doesn't sound good! If you'd like some help with those failing
tests, you could open a new issue.

On Friday, 15 April 2016, kookookchoozeus notifications@github.com wrote:

I will be writing tests for this, but I was getting 11 failed tests on
windows (before I changed anything), so I figured I would take a look at
that while I'm at it, most likely this weekend


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#558 (comment)

@kookookchoozeus
Copy link
Contributor Author

Opened #559, in case the fix is immediately apparent to anyone

var relativePath = path.
relative(templateArg, filename).
replace('.mustache', '').
replace(/\\{1,2}/g, '/');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dots on the left, like pointed out in the other PR.

@dasilvacontin
Copy link
Collaborator

Lacks tests asserting that the feature works. LGTM except for that + the style comments. Thanks!

@kookookchoozeus
Copy link
Contributor Author

Excuse my inexperience with contributing/forks/github in general, but this PR originally was based on the master branch of my fork. Once #559 (fixing tests for Windows) was opened, I made a new branch for that, and did all changes there. That PR was merged, and I merged my local changes (for the relative paths) with the new CLI changes. However, my repo now says it's one commit behind janl:master, so these new changes include all the #559 changes when comparing the two repos. I looked online and found a bunch of stuff on merging with upstream/master, but that caused a lot of conflicts, which I fixed, but I'm not confident I didn't screw something up. Would it be best (and is it even possible) to close my fork now, re-fork from the current janl:master, make my changes, and then submit a new PR? Or is there a better way to go about this?

@dasilvacontin
Copy link
Collaborator

@kookookchoozeus Sorry for the delayed response (bit of a blame on GitHub dismissing desktop notifications when you read the email...)

I got what you mean, I hope I can explain myself as well:

The main problem is that I "squashed & merged", meaning that your commits were taken and combined/smashed into a single new commit, which was put on top of our master branch: 329bfd0.

Which means that when Git compares your local master branch with upstream/master, it notices that particular commit is missing, ie you don't have a commit with that hash (even though you have the same changes through a different set of commits). Hence the "one commit behind". If you try to merge that commit into your branch, you'll ran into conflicts for every single change, since the commit is trying to apply changes from a state that no longer exists into the same state that we currently have.

The better workflow for making multiple PRs, and branches that work on top of other branches (imo and from my experience) is:

  • Don't touch master in your fork, just keep it up to date with upstream/master so that you can easily checkout from it to work on a new feature. You mostly only want to touch your fork's master when you want to use your fork as a replacement for a library (eg maintainers no longer maintain upstream, library is dead, they take long to merge a PR and you need it now, etc). In case you've done that, you can still locally checkout upstream/master and branch from there: git fetch && git checkout upstream/master && git checkout -b your-branch-name.
  • PR always from branches. Delete them once the PR is merged. (you can prune remotely deleted branches via git fetch -p)
  • Now, the hard part, working on a branch B that sits on top of another branch A. branch A's PR gets merged into upstream/master. Now, what you want to do is rebase branch B's work on top of upstream/master. Problem is, in branch B you have branch A's changes (since you were working on top of them), that are now in upstream/master but in different commits (different hash, they were squashed, and maybe they even have fixes / modifications). You need to keep a reference to your branch B commits (like if you cloned them) via branching into branch C from branch B. They are now the same branch except for the name. Now you delete branch B (git branch -D branchB) and you branch from upstream/master into a new branch called branch B. Now branch B is only lacking those commits that were specific of branch B, but that you've "saved" in branch C. How do you put them on top of branch B? You use cherry-pick. Go to branch B and do git cherry-pick 96f23h48d149912384h6 (a commit hash), for all the commits with changes from branch C that we don't have in branch B, from oldest to newest. Lastly, since origin/branchB and your local branchB now have a different history (different set of commits), you'll have to git push -f (force push) in order for the the remote repo to replace the branch's history with the new one (since it can't simply be updated by putting a couple of commits on top of the existing ones).

Let me know if something is not clear 🙈

@dasilvacontin
Copy link
Collaborator

Also, if you don't know about git add -p and git checkout -p, I recommend trying them out. (they let you select which hunks of the file you want to stage / reset)

I also recommend that you have a copy in the repo in case you are new to doing this and you don't want to end up losing code by mistake. Commits can be recovered even if they are not part of a branch anymore, but it's a bit of a hassle.

@kookookchoozeus
Copy link
Contributor Author

Thanks for the writeup :^) It'll serve as a nice reference for similar problems in the future. I've opened a new pull request (#563), so I'll close this one

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.

None yet

3 participants