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

(proposal) Code cleaning for payment method classes. #153

Merged
merged 2 commits into from Mar 10, 2020
Merged

Conversation

guzzilar
Copy link
Contributor

@guzzilar guzzilar commented Feb 27, 2020

1. Objective

This code is a proposal of removing an unnecessary code for registering and listing up Omise payment methods.

Related information:
Related issue(s): T19568 (internal ticket)

2. Description of change

Refactoring part of code that was used for registering Omise payment methods to WooCommerce.

As well as, removing the redundant code.

3. Quality assurance

  1. Making sure that all the payment methods can be listed out at the WooCommerce payment setting page.

  2. Making sure that all the payment methods can be used as normal at WooCommerce checkout page.

4. Impact of the change

None

5. Priority of change

Normal

6. Additional Notes

@guzzilar
Copy link
Contributor Author

guzzilar commented Mar 4, 2020

@jonrandy I removed the whole 'payment methods' class. Can you help check it again?
Thanks :D

@jonrandy
Copy link

jonrandy commented Mar 5, 2020

I think because you 'force pushed' I'm having trouble working out what you've actually removed as those commits are gone. I always find it best never to force push anything - unless it's a last resort to fix some Git weirdness

@guzzilar
Copy link
Contributor Author

guzzilar commented Mar 6, 2020

@jonrandy about force push, you're right. I forgot.
The change I've made is to remove the includes/class-omise-payment-methods.php (the one that you mentioned about singleton) and having a list of payment method classname at omise-woocommerce.php file instead (here: https://github.com/omise/omise-woocommerce/pull/153/files#diff-da400947a51dd0d488fcdb5ac3a4edd4R99).

omise-woocommerce.php Outdated Show resolved Hide resolved
@jonrandy jonrandy self-requested a review March 10, 2020 11:31
'Omise_Payment_Internetbanking',
'Omise_Payment_Truemoney'
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note, I prefer this way instead of constant because it gives a flexibility on altering its payment's classname in the future if we make it extensible. As it's quite common in WordPress world. (i.e. WordPress style is kinda heavily using apply_filter( blah ) to open for a 3rd-party to make a change as they desire without editing the core code).

@guzzilar guzzilar merged commit a57ecc4 into master Mar 10, 2020
@guzzilar guzzilar deleted the code-cleaning branch March 10, 2020 11:34
guzzilar added a commit that referenced this pull request Mar 10, 2020
…e way to register a new payment method).
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

2 participants