Skip to content

[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.

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
Merged
merged 2 commits into from
Sep 30, 2015
Merged

[CssSelector] remove ConverterInterface #15988

merged 2 commits into from
Sep 30, 2015

Conversation

fabpot
Copy link
Member

@fabpot fabpot commented Sep 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

@fabpot fabpot mentioned this pull request Sep 29, 2015
Merged
3 tasks
@fabpot
Copy link
Member Author

fabpot commented Sep 29, 2015

The Converter name is probably not the best one. I'm not a big fan of CssToXPathConverter either and I think the best one is still CssSelector (which implements the static interface). Not sure what to do here. ping @symfony/deciders

@xabbuh
Copy link
Member

xabbuh commented Sep 29, 2015

👍

Status: Reviewed

About the class name: We could merge the Converter and the CssSelector class and only deprecate the static method.

stof
stof reviewed Sep 29, 2015
View reviewed changes
src/Symfony/Component/CssSelector/Converter.php
* This component is a port of the Python cssselect library,
* which is copyright Ian Bicking, @see https://github.com/SimonSapin/cssselect.
*
* Copyright (c) 2007-2012 Ian Bicking and contributors. See AUTHORS
Copy link
Member

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
Member Author

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 fabpot force-pushed the interfaces-deprecation branch from 50e696d to 0256ccc Compare September 29, 2015 08:35
@fabpot
Copy link
Member Author

fabpot commented Sep 29, 2015

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

stloyd
stloyd reviewed Sep 29, 2015
View reviewed changes
src/Symfony/Component/CssSelector/Converter.php
*/
class Converter implements ConverterInterface
class Converter
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@xabbuh
Copy link
Member

xabbuh commented Sep 29, 2015

@fabpot We could rename the new method to asXPath() or something like that or choose a class name like CssTransformer.

@Tobion
Copy link
Contributor

Tobion commented Sep 29, 2015

I think CssSelector 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 just Converter is too generic, as I wouldn't know what the purpose of this class is when I see new Converter().

@weaverryan
Copy link
Member

How about any of CssConverter, CssTransformer or CssTranslator?

@jakzal
Copy link
Contributor

jakzal commented Sep 29, 2015

or CssSelectorConverter ?

@Tobion
Copy link
Contributor

Tobion commented Sep 29, 2015

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

@fabpot fabpot force-pushed the interfaces-deprecation branch 2 times, most recently from fd6c763 to ac17232 Compare September 30, 2015 06:26
@fabpot
Copy link
Member Author

fabpot commented Sep 30, 2015

Renamed and fixed the README to use the new API.

@xabbuh
Copy link
Member

xabbuh commented Sep 30, 2015

👍

@fabpot fabpot force-pushed the interfaces-deprecation branch from 9c3ee7e to 6b02749 Compare September 30, 2015 07:44
@fabpot fabpot force-pushed the interfaces-deprecation branch from 6b02749 to 8b8b634 Compare September 30, 2015 07:45
@fabpot fabpot merged commit 8b8b634 into symfony:2.8 Sep 30, 2015
fabpot added a commit that referenced this pull request Sep 30, 2015
@fabpot
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/a

Commits
-------

8b8b634 [CssSelector] updated README
fd3fefb [CssSelector] remove ConverterInterface
@stof
Copy link
Member

stof commented Sep 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 ?)

@fabpot
Copy link
Member Author

fabpot commented Sep 30, 2015

Indeed, sorry about that, doing a PR right now.

@fabpot
Copy link
Member Author

fabpot commented Sep 30, 2015

see #16012

@xabbuh
Copy link
Member

xabbuh commented Sep 30, 2015

We need to make sure that our testsuite gets back to green asap so that we spot things like this faster. :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@fabpot @xabbuh @Tobion @weaverryan @jakzal @stof @stloyd @carsonbot