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

GRMustacheRenderNSFastEnumeration fix crash handling rendering error #67

Merged
merged 1 commit into from
Feb 3, 2014
Merged

GRMustacheRenderNSFastEnumeration fix crash handling rendering error #67

merged 1 commit into from
Feb 3, 2014

Conversation

nolanw
Copy link
Contributor

@nolanw nolanw commented Feb 3, 2014

I copied the inserted code from other parts of the codebase where similar precautions are necessary (e.g. GRMustacheAccumulatorTag.m).

I included a test, but I'm not sure how the tests folder is organized, or if a test is even desired for this issue. Let me know if I can reorganize things for you.

groue added a commit that referenced this pull request Feb 3, 2014
GRMustacheRenderNSFastEnumeration fix crash handling rendering error
@groue groue merged commit ac216d8 into groue:master Feb 3, 2014
@groue
Copy link
Owner

groue commented Feb 3, 2014

Thank you very much, Nolan! I happily merge your quality PR, and your code is shipped in v6.9.1 😃

I included a test, but I'm not sure how the tests folder is organized, or if a test is even desired for this issue. Let me know if I can reorganize things for you.

Tests are organized by version of the API : tests for APIs introduced in v6.0 go into the v6.0 folder, etc. This sounds pedantic, I know. The reason is that this allows to keep tests for deprecated (yet still supported) methods without getting any compiler warning for using those deprecated methods (see the #define GRMUSTACHE_VERSION_MAX_ALLOWED GRMUSTACHE_VERSION_XXX at the top of each test file). GRMustache API used to be not so stable, and this organization of tests has helped me a lot.

Your test incidentally generates a GRMustacheErrorCodeRenderingError which was introduced in v6.3, so I put it in v6.3.

I appreciate a lot your dedication. Do you want a push access to the repository?

@nolanw nolanw deleted the fix-fastenumeration-error-handling branch February 3, 2014 15:22
@nolanw
Copy link
Contributor Author

nolanw commented Feb 3, 2014

You are very quick!

That organization makes total sense now that you explain it.

Sure, I'll take push access. What would you like me to do with it?

@groue
Copy link
Owner

groue commented Feb 3, 2014

Sure, I'll take push access. What would you like me to do with it?

Well, I'm not sure I know how to answer this question. I really appreciated the quality of your PR, wording & code integration. This is not common... But I don't have any homework to give you :-)

@nolanw
Copy link
Contributor Author

nolanw commented Feb 3, 2014

Well thanks! I was trying to ask about guidelines for contributing, would you prefer PRs from me still, stuff like that.

@groue
Copy link
Owner

groue commented Feb 3, 2014

I only have a forking guide. Do you wish yourself to extend GRMustache for Awful.app ?

@nolanw
Copy link
Contributor Author

nolanw commented Feb 3, 2014

Oh, great! That's what I was looking for. I have no extensions planned, GRMustache works very well.

@groue
Copy link
Owner

groue commented Feb 3, 2014

All right. Well, tell me whenever you need something again :-)

@groue groue added the bug label Feb 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants