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

Fixes wildcard resolving for non-existing paths (issue #814) #815

Merged
merged 1 commit into from Apr 15, 2012

Conversation

HenningJ
Copy link
Contributor

@HenningJ HenningJ commented Apr 13, 2012

Return original string instead of nil if path can't be found during wildcard resolving. Fixes issue #814.

Btw. this is a good example why unit tests are great:
There were unit tests in place for all the intended behavior I knew about. For every change I made, I could just run the unit tests to make sure that I didn't break anything of that.
And to fix the bug, I just added a (failing) test representing the bug. And once the test passed, I knew I had fixed the bug. Without breaking anything else. :-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 15, 2012

Should the new method be declared in the header, and is there a reason at all for the new method? Is it required to have a method that returns nil for some reason and one that returns the original string?

@HenningJ
Copy link
Contributor Author

@HenningJ HenningJ commented Apr 15, 2012

Should the new method be declared in the header

It's a internal/private method, therefore it should not be declared in the public interface/header file. It could be declared in a private category in the implementation file. But since its implementation appears before the first (and only) time it's used, it doesn't need to have a @interface definition. I'm not sure what's the convention here.

and is there a reason at all for the new method? Is it required to have a method that returns nil for some reason and one that returns the original string?

The subStringByResolvingWildcardsInPath recursively calls itself over and over and returns nil on one of its stopping conditions. It could also all be kept in one method and return the original string instead of nil. But I felt this way was easier and probably faster (you'd have to do lots of string comparisons, which are way more expensive than checks against nil). The method that's just a starting point for another recursive method is a pretty common pattern.
I hope that clears it up. :-)

@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 15, 2012

Sure thing, I just wanted a little clarification :)

I know the method's internal/private, but I think we've been declaring
these in the .h files as a convention. Primarily as we're supposed to be
adding doxygem style comments in the .h files... :P

I'll take a look at it so we can try and get a new release out soon before
we get more users commenting :)

On 15 April 2012 10:24, Henning Jungkurth <
reply@reply.github.com

wrote:

Should the new method be declared in the header

It's a internal/private method, therefore it should not be declared in the
public interface/header file. It could be declared in a private category in
the implementation file. But since its implementation appears before the
first (and only) time it's used, it doesn't need to have a @interface
definition. I'm not sure what's the convention here.

and is there a reason at all for the new method? Is it required to have
a method that returns nil for some reason and one that returns the original
string?

The subStringByResolvingWildcardsInPath recursively calls itself over
and over and returns nil on one of its stopping conditions. It could also
all be kept in one method and return the original string instead of nil.
But I felt this way was easier and probably faster (you'd have to do lots
of string comparisons, which are way more expensive than checks against
nil). The method that's just a starting point for another recursive method
is a pretty common pattern.
I hope that clears it up. :-)


Reply to this email directly or view it on GitHub:
#815 (comment)

pjrobertson added a commit that referenced this issue Apr 15, 2012
Fixes wildcard resolving for non-existing paths (issue #814)
@pjrobertson pjrobertson merged commit 74c0bb2 into quicksilver:master Apr 15, 2012
@pjrobertson
Copy link
Member

@pjrobertson pjrobertson commented Apr 15, 2012

Merged, pushed to release.

Looking through it a bit more, I understand the reason for 2 methods now, seems clearer :)

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

2 participants