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.

Sign up for GitHub

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

Jump to bottom

Allow to pass a charset to the Scanner #163

Merged
merged 3 commits into from
Feb 22, 2019
Merged

Allow to pass a charset to the Scanner #163

merged 3 commits into from
Feb 22, 2019

Conversation

tgalopin
Copy link
Contributor

@tgalopin tgalopin commented Dec 8, 2018 •
edited
Loading

Also fixes a few comments indentation.

@goetas
Copy link
Member

goetas commented Dec 9, 2018

Hi, instead of adding a new parameter, why not add an option to the option array?
It will be consistent with the rest of the public api

@tgalopin
Copy link
Contributor Author

tgalopin commented Dec 9, 2018

Okay, no problem :) .

@goetas
Copy link
Member

goetas commented Dec 12, 2018

@tgalopin let me know when this is done, so i can create a new release with the recent performance improvements

@tgalopin
Copy link
Contributor Author

Sorry, working on it, the SymfonyCon Lisbon was tiring :) .

@tgalopin
Copy link
Contributor Author

Done :) !

goetas
goetas reviewed Dec 12, 2018
View reviewed changes
Copy link
Member

@goetas goetas left a comment

Choose a reason for hiding this comment

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

currently the order of mb/iconv was intentional as mb is faster than iconv

@tgalopin
Copy link
Contributor Author

I didn't change this, I just expanded the conditionnals using only ifs instead of elseifs :) .

@goetas
Copy link
Member

goetas commented Dec 12, 2018

Right! I should stop making code reviews using the phone! 😄

@goetas
Copy link
Member

goetas commented Dec 12, 2018

Can you add some tests for this feature?

@tgalopin tgalopin force-pushed the charset-support branch 2 times, most recently from 17eaf25 to 8a15a3e Compare December 29, 2018 15:52
@tgalopin
Copy link
Contributor Author

I added tests :) .

@goetas
Copy link
Member

goetas commented Jan 2, 2019

hmm, it looks something did not work

@tgalopin
Copy link
Contributor Author

tgalopin commented Feb 6, 2019 •
edited
Loading

I updated the tests :) .

EDIT: and they still fail ... :D working on it.

@tgalopin
Copy link
Contributor Author

tgalopin commented Feb 6, 2019

Done \o/

@tgalopin tgalopin mentioned this pull request Feb 21, 2019
Merged
@tgalopin
Copy link
Contributor Author

I think this is ready for review :)!

@goetas goetas merged commit 92fff5c into Masterminds:master Feb 22, 2019
@goetas
Copy link
Member

goetas commented Feb 22, 2019

Thanks!

@goetas
Copy link
Member

goetas commented Feb 22, 2019

Do you need a tag for it or can wait?

@tgalopin
Copy link
Contributor Author

A tag would be nice, but it can wait a few days/weeks if needed :) .

@tgalopin tgalopin deleted the charset-support branch February 22, 2019 09:46
@goetas
Copy link
Member

goetas commented Mar 10, 2019

released as 2.6.0

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

@goetas goetas goetas left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tgalopin @goetas