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

Feature/fileutils refactor cleanup #584

Closed
wants to merge 43 commits into from

Conversation

ambs
Copy link
Member

@ambs ambs commented Jun 26, 2011

This is sawyer PR. Just making it public for better analysis, and not to forget.

@ambs
Copy link
Member Author

ambs commented Jun 26, 2011

Tested under mac OS X (Leopard) and Windows XP (Strawberry Perl). Both a sucess. "approved"

@xsawyerx
Copy link
Member

Yay! Will test it on Windows again before merging.

@mokko
Copy link
Contributor

mokko commented Jun 27, 2011

How can I check it out to test it on cygwin? EDIT: Now I found it. Passes tests on cygwin. Code looks mighty clean!

@ambs
Copy link
Member Author

ambs commented Jun 27, 2011

git clone git://github.com/sukria/Dancer.git
git fetch origin 
git checkout feature/fileutils_refactor_cleanup

@mokko
Copy link
Contributor

mokko commented Jun 27, 2011

I don't know why I didn't see the branch feature/fileutils_refactor_cleanup at first. Maybe it didn't exist then or i was just blind. Anyways, Tests pass now on my cygwin. (It didn't do all the Test::TCP etc. tests.)

I wonder whether we still need path_no_verify. It still has a call to d_catdir and that is gone, right?

@ambs
Copy link
Member Author

ambs commented Jun 27, 2011

And if tests pass, then probably it is not used :P

@xsawyerx
Copy link
Member

@mokko, I just added it a few days ago.

I'm happy all the tests pass on Cygwin! Thanks for testing it!
There is no path_no_verify() now. The regular path() function no longer verifies so it is the same as path_no_verify(), just MUCH faster and cleaner.

I added a wedge function that will be deprecated called path_or_empty() to return the path or empty string on non-existent paths. I'll deprecate it in the future in favor of checking if the path exists and then throwing an error if not.

It appears it passes all, so now we can collect approvals and merge it in.

@mokko, thanks again for your input on this entire subject!

@sukria
Copy link
Member

sukria commented Jun 28, 2011

approved! Thanks @xsawyerx for the great work!

@fcuny
Copy link
Contributor

fcuny commented Jun 28, 2011

approved. I'm not sure why, but some commits are present more than once (some are presents 3 times)

@ambs
Copy link
Member Author

ambs commented Jun 28, 2011

merged. Thanks

@ambs ambs closed this Jun 28, 2011
@xsawyerx
Copy link
Member

@franckcuny, I had issues rebasing it. Some really weird behavior... @sukria couldn't really help me understand it either. :)

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.

5 participants