NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[CssSelector] remove ConverterInterface#15988

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 2 commits intosymfony:2.8fromfabpot:interfaces-deprecation

Sep 30, 2015

Merged

[CssSelector] remove ConverterInterface#15988

fabpot merged 2 commits intosymfony:2.8fromfabpot:interfaces-deprecation

Sep 30, 2015

Conversation

Copy link

Member

fabpot commentedSep 29, 2015

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#15934
License	MIT
Doc PR	n/a

carsonbot added the Status: Needs Review label

Sep 29, 2015

fabpot mentioned this pull request

Sep 29, 2015

Add a non-static API for the CssSelector component#15934

Merged

3 tasks

Copy link

MemberAuthor

TheConverter name is probably not the best one. I'm not a big fan ofCssToXPathConverter either and I think the best one is stillCssSelector (which implements the static interface). Not sure what to do here. ping @symfony/deciders

Copy link

Member

xabbuh commentedSep 29, 2015

👍

Status: Reviewed

About the class name: We could merge theConverter and theCssSelector class and only deprecate the static method.

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Sep 29, 2015

stof reviewed

Sep 29, 2015

View reviewed changes

src/Symfony/Component/CssSelector/Converter.php Outdated

Copy link

Member

stofSep 29, 2015

Choose a reason for hiding this comment

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

@fabpot is the phpdoc the right place to put the license of the ported library ? This class is not even ported from the Python library (the classes porting the Pythin library are all the internal classes)

Copy link

MemberAuthor

fabpotSep 29, 2015

Choose a reason for hiding this comment

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

moved to the README file instead

fabpot force-pushed theinterfaces-deprecation branch from50e696d to0256cccCompare

September 29, 2015 08:35

Copy link

MemberAuthor

fabpot commentedSep 29, 2015

@xabbuhtoXPath() are in both the new and the old class (one is static, the other one is not).

stloyd reviewed

Sep 29, 2015

View reviewed changes

src/Symfony/Component/CssSelector/Converter.php Outdated

Copy link

Contributor

stloydSep 29, 2015

Choose a reason for hiding this comment

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

Maybe this should be calledTransformer? As this transforms one convention to another ;)

Copy link

Member

xabbuh commentedSep 29, 2015

@fabpot We could rename the new method toasXPath() or something like that or choose a class name likeCssTransformer.

Copy link

Contributor

Tobion commentedSep 29, 2015

I thinkCssSelector would be the wrong name for a non-static class. In OOP I would image an API of a CssSelector like(new CssSelector($css))->toXPath(). So the CssSelector represents a single css expression. But that is not the API we have here right now. So Converter/Transformer (but more specific) makes alot more sense, as you only need one converter to transform alot of css expressions.

But again justConverter is too generic, as I wouldn't know what the purpose of this class is when I seenew Converter().

Copy link

Member

weaverryan commentedSep 29, 2015

How about any ofCssConverter,CssTransformer orCssTranslator?

Copy link

Contributor

jakzal commentedSep 29, 2015

orCssSelectorConverter ?

Copy link

Contributor

Tobion commentedSep 29, 2015

+1 forCssSelectorConverter as we are converting a selector and not css code

fabpot force-pushed theinterfaces-deprecation branch 2 times, most recently fromfd6c763 toac17232Compare

September 30, 2015 06:26

Copy link

MemberAuthor

fabpot commentedSep 30, 2015

Renamed and fixed the README to use the new API.

Copy link

Member

xabbuh commentedSep 30, 2015

👍

[CssSelector] remove ConverterInterface

fd3fefb

fabpot force-pushed theinterfaces-deprecation branch from9c3ee7e to6b02749Compare

September 30, 2015 07:44

[CssSelector] updated README

8b8b634

fabpot force-pushed theinterfaces-deprecation branch from6b02749 to8b8b634Compare

September 30, 2015 07:45

fabpot merged commit8b8b634 intosymfony:2.8

Sep 30, 2015

fabpot added a commit that referenced this pull request

Sep 30, 2015

minor#15988[CssSelector] remove ConverterInterface (fabpot)

c06db30

This PR was merged into the 2.8 branch.Discussion----------[CssSelector] remove ConverterInterface| Q             | A| ------------- | ---| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#15934| License       | MIT| Doc PR        | n/aCommits-------8b8b634 [CssSelector] updated READMEfd3fefb [CssSelector] remove ConverterInterface

Copy link

Member

stof commentedSep 30, 2015

This is broken. You forgot to use the new class name in the DomCrawler component (why do we have a red flag on Travis and AppVeyor if nobody look at the test failures ?)