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

[Product] VariantFactory naming changes #4065

Merged
merged 2 commits into from
Feb 17, 2016

Conversation

Zales0123
Copy link
Member

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets
License MIT
Doc PR
  • changed VariantFactory into ProductVariantFactory, as it's placed in Product component and creates only ProductVariant objects
  • changed createForProduct method into createBasedOnProductId
  • change invalid parameter names in factory and factory interface

@pjedrzejewski pjedrzejewski added Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). RFC Discussions about potential changes or new features. labels Feb 5, 2016
{
if (null === $product = $this->productRepository->find($productId)) {
throw new \InvalidArgumentException(sprintf('Product with id "%s" does not exist.', $productId));
if (null === $product = $this->productRepository->find($id)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we extract the assignment before the if?

@pjedrzejewski
Copy link
Member

  1. Not sure about the renaming of the factory, currently in Product component we have https://github.com/Sylius/Product/blob/master/Model/Variant.php model, not ProductVariant, because it seemed too obvious to say it for all other models. ProductVariant, ProductAttributeValue, etc. We actually have a mismatch there right now, since introducing ProductAssociation.

So we should first agree on that convention and then rename the factory if needed. I'd vote for leaving out the Product prefix here, cause it seems to be duplicated when we have Product in the namespace already. If we ever end up with 2 Variant concepts in that namespace (I doubt) then it is a much bigger problem than prefixing/not-prefixing stuff.

  1. createBasedOnProductId does not sound really good to me... :(

createForProductId
createForProductWithId

Saying "Create variant based on product id" may sound reasonable but in reality we are "Creating a variant of a product with ID", but createOfProductWithId sounds terrible too... :D

@Zales0123
Copy link
Member Author

  1. Ok, that sound reasonable for me 👍
  2. createForProductWithId looks the more appropriate for me (``I create variant for product with id "$id")

@pjedrzejewski
Copy link
Member

Let's hear few more opinions on the naming because we will go with that convention for other factories as well. :)

@pamil
Copy link
Contributor

pamil commented Feb 8, 2016

I'm for adding subject to the name of generic classes like Variant as soon as they are aware of it. Like our Sylius\Component\Product\Model\Variant has getProduct() and setProduct() methods, therefore it should be named ProductVariant. If it would not have them, it should be still named Variant.

@Zales0123
Copy link
Member Author

Ok, so finally after hours of discussion we decided to stay with ProductVariantFactory class and createForProductWithId method ;)

pjedrzejewski pushed a commit that referenced this pull request Feb 17, 2016
[Product] VariantFactory naming changes
@pjedrzejewski pjedrzejewski merged commit bae949c into Sylius:master Feb 17, 2016
@pjedrzejewski
Copy link
Member

Thanks Mateusz! 👍

@Zales0123 Zales0123 deleted the variant-factory-fixes branch October 28, 2016 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). RFC Discussions about potential changes or new features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants