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

Move WC_GoCardless class to its own file #1

Open
leonardola opened this issue Oct 10, 2022 · 4 comments
Open

Move WC_GoCardless class to its own file #1

leonardola opened this issue Oct 10, 2022 · 4 comments
Labels
priority: low The issue/PR is low priority—not many people are affected or there’s a workaround, etc. type: technical debt This issue/PR represents/solves technical debt of the project.

Comments

@leonardola
Copy link

Description

I would be nice to have a separate file for the WC_GoCardless class instead of using the main plugin file for it.

@leonardola leonardola added priority: low The issue/PR is low priority—not many people are affected or there’s a workaround, etc. type: technical debt This issue/PR represents/solves technical debt of the project. labels Oct 10, 2022
@ravinderk ravinderk self-assigned this Oct 31, 2022
@ravinderk
Copy link

@leonardola I notices that this file has logic to do following:

I think main plugin file should only bootstrap plugin. I did not see need of refactoring this file at this moment or benifit.
I think we should stop adding code to this files except if we are loading a new file.

Let me know if you have any question or you want me to implement refactoring for above list except loading files logic.

@jeffpaul jeffpaul added the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Oct 31, 2022
@dkotter
Copy link
Collaborator

dkotter commented Nov 3, 2022

I think main plugin file should only bootstrap plugin.

I do agree with this in most cases, though looking at various other Woo extensions, there doesn't seem to be a consistent approach. Some have a class in the main plugin file like this extension, others don't but still have lots of functionality in the main plugin file. Doesn't mean we can't/shouldn't clean this extension up though.

I did not see need of refactoring this file at this moment or benefit.

@ravinderk Just to be clear, you're suggesting we hold off on doing any sort of refactoring at the moment, correct? If so, I think that's fine though I would like to keep this item in our backlog and we can look to tackle this if we don't have any higher priority items.

@ravinderk
Copy link

I think main plugin file should only bootstrap plugin.

I do agree with this in most cases, though looking at various other Woo extensions, there doesn't seem to be a consistent approach. Some have a class in the main plugin file like this extension, others don't but still have lots of functionality in the main plugin file. Doesn't mean we can't/shouldn't clean this extension up though.

I did not see need of refactoring this file at this moment or benefit.

@ravinderk Just to be clear, you're suggesting we hold off on doing any sort of refactoring at the moment, correct? If so, I think that's fine though I would like to keep this item in our backlog and we can look to tackle this if we don't have any higher priority items.

@dkotter Yes, I am suggesting to hold off at this moment.

@leonardola
Copy link
Author

I'm ok with holding off. That's why it's low priority. It's something we should aim but not something we need to do now.

@dkotter dkotter removed the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Nov 3, 2022
@diegocurbelo diegocurbelo transferred this issue from another repository Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low The issue/PR is low priority—not many people are affected or there’s a workaround, etc. type: technical debt This issue/PR represents/solves technical debt of the project.
Projects
None yet
Development

No branches or pull requests

4 participants