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

quote packageDirectory to handle package-names which contain spaces #140

Merged
merged 1 commit into from
Feb 1, 2015

Conversation

eMBee
Copy link

@eMBee eMBee commented Feb 1, 2015

this is my first attempt at a contribution via gitfiletree. should i be removing all those property changes? it looks very messy.

@dalehenrich
Copy link
Owner

@eMBee, thanks for your contribution!

The tests with your changes aren't passing for Pharo4.0:

**************************************************************************************
    Results for #('BaselineOfFileTree') Test Suite
46 run, 45 passes, 0 skipped, 0 expected failures, 0 failures, 0 errors, 1 unexpected passes
**************************************************************************************
*** FAILURES *******************
    MCFileTreeLoaderTraitsTest debug: #testLoad.
**************************************************************************************

Pharo4.0 is a bit of a moving target these days, so it isn't always obvious if the test failure is due to your changes or changes in Pharo4.0 that are independent of your changes ... I had run a test earlier today with some of my own changes that failed running Pharo4.0, so I am suspicious that the root problem is in Pharo4.0 ...

Since the changes are in @ThierryGoubier code, I will leave it up to him to review your work with relation to the failed tests ...

At the end of the day, I think may end up creating a Pharo4.0 branch if changes need to be made on the filetree side of things ...

@ThierryGoubier
Copy link
Collaborator

Hi @eMBee, the property changes are expected, don't worry about them. I'll have a look on the tests part of things.

@dalehenrich
Copy link
Owner

Oops @ThierryGoubier, I inadvertantly restarted the pull request trvis job ... meant to restart my failed tests ... sorry

@dalehenrich
Copy link
Owner

@ThierryGoubier I'm tracking a slightly different problem with Pharo4.0 for my work on Issue #136 ... I will wait until you are done merging this pull request into the pharo3.0 before merging my work ... I might end up having to create a separate pharo4.0 branch, dependeing upon what I find...hopefully I won't have to...

@ThierryGoubier
Copy link
Collaborator

Ok, the failing test is an unexpected pass :)

We need a pharo4.0 branch and correct there, otherwise we won't get a green travis. Or I merge this on the pharo3.0 branch first? It's green on 3.0 and we start the work on the pharo4.0 branch from that point.

@dalehenrich
Copy link
Owner

@ThierryGoubier I see that this is a pragma, if the #expectedFailures method still works for unit tests,then we don't need a new branch ... yet ... I'm tracking another Pharo4.0 issue, so I will take care of this one way or the other ... if you are comfortable with this change go ahead and complete the merge and I will deal with the pharo3.0 vs pharo4.0 branch separately ...

ThierryGoubier added a commit that referenced this pull request Feb 1, 2015
quote packageDirectory to handle package-names which contain spaces

pharo3.0 tests are green.
@ThierryGoubier ThierryGoubier merged commit 3591abd into dalehenrich:pharo3.0 Feb 1, 2015
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