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

Fixed EZP-20029: Refactor XmlText so that input value content is always ... #131

Closed
wants to merge 1 commit into from

Conversation

patrickallaert
Copy link
Contributor

...validated

Removing Input Converters with Input value objects.
Input\EzXml is internally used by the XmlText\Type in case of XML string
content being given.

@@ -17,7 +17,7 @@
/**
* Converts internal
*/
class Html5 implements Output
class Html5 implements Converter
Copy link
Contributor

Choose a reason for hiding this comment

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

With "Output" out, the name should probably specify to or from info, like Html5ToEzxml or opposite if that is the case.

Alternatively interface could cover both directions, unsure about the naming of the methods though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While rethinking XmlText, I thought that with the new design, it was more obvious that an XmlText\Converter would convert an XmlText and as such XmlText\Converter\Html5 would simply convert XmlText to Html5.

If that isn't obvious, I will change it, but I don't really have a good alternative since to me XmlText\ConverterTo\Html5 doesn't really solve the problem since it can be interpreted as "Html5 being ConvertedTo XmlText" or the opposite way around as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is obvious now that I know your reasoning, but it sure was not obvious when I saw this file on it's own.
But clarifying it with more class doc on some of the files in this PR will help more then enough :)

@andrerom
Copy link
Contributor

A bit unsure what Input means now, as Output is not Converter.

( seems like you need to rebase this branch or something, some extra commits are part of the PR, including comments )

@emodric
Copy link
Contributor

emodric commented Nov 12, 2012

@andrerom Happened to me too on my PR and I can't get rid of it, as rebase does not help :/

@andrerom
Copy link
Contributor

Ok, then this needs to be checked closer, since when I merged your PR we got notifications in campfire about lots of commits being pushed (old + existing commits), so there might be something fishy here.

@dpobel
Copy link
Contributor

dpobel commented Nov 12, 2012

it's probably my fault :-/ I did a force push this morning by mistake. To fix it, I cherry picked missing commits from Petar... that's why, Petar's commits appear here and in the Edit's pull request... I'm really sorry for this

@emodric
Copy link
Contributor

emodric commented Nov 12, 2012

@andrerom I was referring to the new one #132, but yeah...

Tried two approaches without luck:

  1. rebasing original branch and force pushing
  2. creating new one on top of current master, cherry-picking my commits and force pushing

@andrerom
Copy link
Contributor

ok, if dp did a force push on master then that explains it, so lets remind of an old rule:

  • Never force push against anything but topic branches

…ys validated

Removing Input Converters with Input value objects.
Input\EzXml is internally used by the XmlText\Type in case of XML string
content being given.
@patrickallaert
Copy link
Contributor Author

To make this PR clean again, I just dit the following:

$ git pull --rebase (while on master branch)
$ git checkout EZP-20029
$ git rebase master
$ git push --force patrickallaert EZP-20029

@emodric
Copy link
Contributor

emodric commented Nov 12, 2012

@patrickallaert Thanks, that did it. I have no idea why step 1 is necessary though :)

@lolautruche
Copy link
Contributor

👍 Looks ok to me.

@andrerom
Copy link
Contributor

+1

@patrickallaert
Copy link
Contributor Author

PR has been manually merged while adding some documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants