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

[DB] Remove COMMENT database configuration #202

Merged
merged 1 commit into from
Apr 10, 2020

Conversation

lchrusciel
Copy link
Member

No description provided.

@lchrusciel lchrusciel requested a review from a team as a code owner March 12, 2020 12:37
@diimpp
Copy link
Member

diimpp commented Mar 16, 2020

I would vote to drop this ENUM thing entirely. Isn't used in Sylius, so no reason to use over here.

@diimpp
Copy link
Member

diimpp commented Mar 16, 2020

@lchrusciel Would you agree to drop ENUM package? I can add PR for that later on.

@lchrusciel
Copy link
Member Author

I'm in favor of it, but I would like to gather the second vote here. /cc @Zales0123

@Zales0123
Copy link
Member

Zales0123 commented Apr 8, 2020

Dropping the enum -> simplification & consistency -> profit 👌

@Zales0123 Zales0123 added DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.). labels Apr 8, 2020
Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

We can drop the enum in the separate PR, I suppose 👌

@Zales0123 Zales0123 merged commit 8e3cb42 into Sylius:master Apr 10, 2020
@Zales0123
Copy link
Member

Thanks, Łukasz! 🎉

@lchrusciel lchrusciel deleted the postgres-support branch April 10, 2020 08:42
@lchrusciel
Copy link
Member Author

@diimpp do you think, you could find time to work on this enum removal? I would like to publish a new version of RefundPlugin and this is one of the changes, that could be included

@diimpp
Copy link
Member

diimpp commented Apr 10, 2020

@lchrusciel yes, but I think state machine code for Order->state==refunded is broken (Maybe not true, not really tested by me) and there is matter of Refund/RefundPayment/CreditMemo design issue. It's on lower priority for me, but I will check it tomorrow and let you know if I can do it early.

@lchrusciel
Copy link
Member Author

Therefore, I will probably publish the current state as it is, as some useful refactorings were done already.

@diimpp
Copy link
Member

diimpp commented Apr 13, 2020

@lchrusciel, it look like a hefty task, many method signatures using this RefundType enum, 41 entry in the source code.

So I would be better to go ahead with my other refactor first and look at this afterwards. Some of the classes, that are using this enum may change drastically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX Issues and PRs aimed at improving Developer eXperience. Enhancement Minor issues and PRs improving the current solutions (optimizations, typo fixes, etc.).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants