Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Adding AoP around advice examples. Fixes #114 #115

Merged
merged 1 commit into from
Jul 10, 2013

Conversation

skiadas
Copy link
Contributor

@skiadas skiadas commented Jun 20, 2013

No description provided.

@briancavalier
Copy link
Member

Wow, @skiadas, thanks for this! I'll try to review in the next day or so.

@skiadas
Copy link
Contributor Author

skiadas commented Jun 23, 2013

This latest commit should also fix #116

@briancavalier
Copy link
Member

Left one comment about the around example, but otherwise this looks great. Let's work through the around advice, and then I'll get this merged. Thanks!

@skiadas
Copy link
Contributor Author

skiadas commented Jun 24, 2013

Let's try this one. I left out proceedCount, as I didn't think it was the most critical component of "around".

@briancavalier
Copy link
Member

Looks good. Yeah, I think leaving out proceedCount is the right thing. It's fairly specific to meld (as is the joinpoint, unfortunately, although the concept of a joinpoint is quite general).

Thanks again for this. I'll merge it today.

@skiadas
Copy link
Contributor Author

skiadas commented Jul 9, 2013

@briancavalier where are we with this pull request? Do I need to add/change anything?

@briancavalier
Copy link
Member

Whoops! Sorry I let this drop. Looking at it now

@skiadas
Copy link
Contributor Author

skiadas commented Jul 9, 2013

Okay something really weird is going on. I have created a new commit which I according to my computer have pushed, and it is here: skiadas@21caf31
And it's supposed to be part of the docs-fixes branch. But somehow it is not shown when I view the branch, and it's not show in the pull request.

@briancavalier
Copy link
Member

Hrm, not sure, but I've managed to get into (and out of, thankfully) similar situations. Sounds like you might try rebasing your branch on latest upstream master, then interactive rebasing to squash all your commits for this PR into a single commit, then force pushing that to your remote branch. Takes a little git-fu, but there are some good refs here, here, and here.

Another option would be to create a new branch from upstream/master, then git merge --squash your doc-fixes branch into the new branch, commit the result and git push --force it to your remote doc-fixes branch.

Hope that helps, and keep me posted!

Add 'around' example in wire spec example
Add example of a 'around' method implementation
Use destroy instead of ready in the example for destroy
Updated "around" example
Simplified around advice example
@skiadas
Copy link
Contributor Author

skiadas commented Jul 10, 2013

Phew, all this rebasing will take some more time to master, but I think it worked?

@briancavalier
Copy link
Member

Yeah, rebasing/squashing/force pushing is pretty radical if you've not done it before :) Once you get the hang of it, it's incredibly useful, especially for pull requests.

Looking over the result, it all looks good. Thanks for working through this!

briancavalier added a commit that referenced this pull request Jul 10, 2013
Adding AoP around advice examples. Fixes #114
@briancavalier briancavalier merged commit 08ca0b7 into cujojs:master Jul 10, 2013
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants