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

Already on GitHub? Sign in to your account

Name generation fix #141

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

b00giZm commented Oct 18, 2011

See issue #132

Best wishes! ;)

Collaborator

stof commented Feb 28, 2012

@kriswallsmith is there an issue with this bugfix or did you simply miss it 4 months ago ?

Owner

kriswallsmith commented Feb 28, 2012

I'd like to see a test to prove this fix. Otherwise we're just tinkering.

Owner

kriswallsmith commented Feb 28, 2012

It also needs to be rebased to get rid of the merge commit.

b00giZm commented Feb 28, 2012

I get you point, but it I just switched two statements and writing a test for that would be a lot overhead.

Instead, wouldn't it be sufficient to prove that this change doesn't break any existing tests?

Owner

kriswallsmith commented Feb 28, 2012

This is an obscure aspect of Assetic that should be covered by the test suite.

Collaborator

stof commented Feb 28, 2012

@b00giZm the issue is that the previous implementation did not break the test either. So you should add a new test for the bug you fix, which should be failing with the previous implementation and should pass with your fix. Otherwise, nothing would avoid a regression later

b00giZm commented Feb 28, 2012

Okay guys, I'll look into it, when I find time ;)

Collaborator

schmittjoh commented Mar 1, 2012

Please just open another PR if you have something ready for review.

@schmittjoh schmittjoh closed this Mar 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment