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

[CurrencyBundle] Add command to allow updating exchange rates using ECB database #1831

Merged
merged 1 commit into from
Sep 3, 2014

Conversation

stloyd
Copy link
Contributor

@stloyd stloyd commented Aug 25, 2014

ECB = European Central Bank

@pjedrzejewski
Copy link
Member

I think we need something more generic... Command is fine, but it should be able to use different adapters. Exchange rates updating service sounds like a solution to me. What do you think?

@stloyd
Copy link
Contributor Author

stloyd commented Aug 25, 2014

@pjedrzejewski I agree... but currently this is one step forward where we don't have any real option to handle this ;)

If you give me some example database that can be used instead of ECB I will try to add them as more generic case, otherwise I think it's ok for now, and make it more generic when we have usecase (other databases) to handle.

@kayue
Copy link
Contributor

kayue commented Aug 26, 2014

@pjedrzejewski
Copy link
Member

^ this. I just think it can be really simple implementation with adapters or providers. :)

@stloyd stloyd mentioned this pull request Aug 31, 2014
@stloyd stloyd changed the title [WIP][CurrencyBundle] Add command to allow updating exchange rates using ECB database [CurrencyBundle] Add command to allow updating exchange rates using ECB database Sep 1, 2014
@stloyd
Copy link
Contributor Author

stloyd commented Sep 1, 2014

@pjedrzejewski @kayue @Arn0d I guess it's ready for first review.

@kayue
Copy link
Contributor

kayue commented Sep 1, 2014

Personally I prefer full name instead of OERImporter, other than that it looks good to me 👍

Thanks @stloyd .

@stloyd
Copy link
Contributor Author

stloyd commented Sep 1, 2014

@kayue For me both versions are ok, so I can give full names and add aliases for short names.

@stloyd stloyd force-pushed the feature/currency_command branch 2 times, most recently from 671cdfd to 87e1393 Compare September 2, 2014 08:14
@stloyd
Copy link
Contributor Author

stloyd commented Sep 2, 2014

@Arn0d @kayue @pjedrzejewski Anything missing for this?

pjedrzejewski pushed a commit that referenced this pull request Sep 3, 2014
[CurrencyBundle] Add command to allow updating exchange rates using ECB database
@pjedrzejewski pjedrzejewski merged commit a60f4f3 into Sylius:master Sep 3, 2014
@pjedrzejewski
Copy link
Member

Looks like a good start to me! I agree with Ka Yue for both points, I think we should improve it with tagged services and use full names.

Thank you very much Joseph! 👍

@stloyd stloyd deleted the feature/currency_command branch September 3, 2014 13:10
@Djuki
Copy link

Djuki commented Sep 3, 2014

@pjedrzejewski My team and I have worked on a similar feature. We have Multiple Exchange Rate Providers (ECB, Google, Yahoo)

Also we have cli Command for updating all rates. We have phpspec included.

Is it too late for us to send PR? Perhaps you could take a look?

I would like to hear your opinion about our approach and work. I know you accepted this PR but maybe it is not too late to take a look at another similar PR.

@stloyd
Copy link
Contributor Author

stloyd commented Sep 3, 2014

@Djuki I guess there is no problem to adapt your code to merged one ;) So feel free to create PR :)

@pjedrzejewski
Copy link
Member

@Djuki Yes, please go ahead and send the PR! 👍 I'd love to see how can we use it!

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
[CurrencyBundle] Add command to allow updating exchange rates using ECB database
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants