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

Refactor #8

Merged
merged 6 commits into from Oct 11, 2014
Merged

Refactor #8

merged 6 commits into from Oct 11, 2014

Conversation

snapshotpl
Copy link
Contributor

Remove static, factories, remove extend in filter, extends service providers and more test

$this->slugifier = $slugifier;
}

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually wondering… do we need this getter at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In module we don't use it, but someone want to get it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really see any reason for it. After all, they may get it through the service manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you want to check slugifier in filter? You don't see reason to keep it, I don't see reason to remote it.

@DASPRiD
Copy link
Member

DASPRiD commented Apr 9, 2014

Also, make sure to fix the build.

@snapshotpl
Copy link
Contributor Author

@DASPRiD Travis fail on a test. Can you restart it manually?

@DASPRiD
Copy link
Member

DASPRiD commented Apr 9, 2014

Actually PHPCS fails.

@snapshotpl
Copy link
Contributor Author

I have pushed new commit 'Fix coding standard', but test failed on composer

@DASPRiD
Copy link
Member

DASPRiD commented Apr 9, 2014

I see. Well, I cannot restart travis on pull requests.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 30a6faf on snapshotpl:refactor into 118e9d8 on Bacon:master.

@snapshotpl
Copy link
Contributor Author

@DASPRiD Green light :-)

@snapshotpl
Copy link
Contributor Author

@DASPRiD should I fix something more?

*
* @link http://github.com/Bacon/BaconStringUtils For the canonical source repository
* @copyright 2011-2014 Ben Scholzen 'DASPRiD'
* @author Witold Wasiczko <witold@wasiczko.pl>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use author tags :)

@DASPRiD DASPRiD merged commit 30a6faf into Bacon:master Oct 11, 2014
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