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

UppercaseRot13Transformer wrong class name used #6714

Closed
wants to merge 3 commits into from

Conversation

jevgenijusr
Copy link

@jevgenijusr jevgenijusr commented Jul 4, 2016

Regarding UppercaseRot13Transformer: please double check this, because I suppose UppercaseTransformer has to be used instead. We are autowiring UppercaseTransformer, also there is no description of UppercaseRot13Transformer at all. P.S. If I am wrong, then I apologize 100 times:), but in any case this means it can be difficult to understand part of the recipe

Regarding UppercaseRot13Transformer please double check this, because I suppose UppercaseTransformer has to be used instead. We are autowiring UppercaseTransformer, also there is no description of UppercaseRot13Transformer at all.
@@ -317,7 +317,7 @@ and a Twitter client using it:
autowire: true

uppercase_rot13_transformer:
Copy link
Member

Choose a reason for hiding this comment

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

I think we should rename this to uppercase_transformer.

@xabbuh
Copy link
Member

xabbuh commented Jul 4, 2016

@jevgenijusr This change looks good to me. Can you also update the following code blocks for the XML and PHP formats accordingly?

Removed usage of uppercase_rot13_transformer, used uppercase_transformer instead. Changed in YAML, XML and PHP code blocks.
@xabbuh
Copy link
Member

xabbuh commented Jul 4, 2016

👍

Status: Reviewed

@javiereguiluz
Copy link
Member

javiereguiluz commented Jul 4, 2016

@jevgenijusr this looks like a bug. Thanks for fixing it! However, shouldn't we use Rot13Transformer instead of UppercaseTransformer ? That's what's used in the first part of the article.

Changed name of UppercaseTransformer to UppercaseRot13Transformer because it makes more sense: in this part of the article we do either rot13 transform or uppercased rot13 transform, therefore we use Rot13Transformer and UppercaseRot13Transformer. Actually the only problem this article had before this PR is it had mispelled "class UppercaseTransformer" (line 238) , it should have been "class UppercaseRot13Transformer", I renamed everything to use UppercaseTransformer at first, but best name would be UppercaseRot13Transformer (smallest change too) :) Take a look, thanks.
@jevgenijusr
Copy link
Author

@javiereguiluz Thanks, I think we should use UppercaseRot13Transformer instead, small change in the end :)

wouterj added a commit that referenced this pull request Jul 5, 2016
This PR was submitted for the 3.0 branch but it was merged into the 2.8 branch instead (closes #6714).

Discussion
----------

UppercaseRot13Transformer wrong class name used

Regarding UppercaseRot13Transformer: please double check this, because I suppose UppercaseTransformer has to be used instead. We are autowiring UppercaseTransformer, also there is no description of UppercaseRot13Transformer at all. P.S. If I am wrong, then I apologize 100 times:), but in any case this means it can be difficult to understand part of the recipe

Commits
-------

afa8e0e UppercaseRot13Transformer wrong class name used
@wouterj
Copy link
Member

wouterj commented Jul 5, 2016

Hi @jevgenijusr! Thanks for fixing this bug. The article is already quite complex, let alone when there is a typo in it :)

I've merged your PR in the 2.8 version of the docs, as this feature was added in 2.8. I'll merge it in the newer versions from there. After merging, I decided to rename UppercaseRot13Transformer to UppercaseTransformer: The class is actually a decorator that simply uppercases the value of any transformer passed in the constructor. You can see this change (among some other changes) in 048b853

Thanks again!

@wouterj wouterj closed this Jul 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants