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

Revert "New event so more validation classes can be added on the fly" #2910

Merged
merged 1 commit into from
Jan 9, 2023

Conversation

fballiano
Copy link
Contributor

We were still discussing #1490, there were a few points that should have been addressed before merging.

I think the merge of #1490 into branch 20.0 should be reverted and the whole PR re-discussed.

@github-actions github-actions bot added the Component: Eav Relates to Mage_Eav label Jan 9, 2023
Copy link
Contributor

@Flyingmana Flyingmana left a comment

Choose a reason for hiding this comment

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

Blocked because there is no reason named how this changes has a negative effect

@fballiano fballiano dismissed Flyingmana’s stale review January 9, 2023 00:06

it is literally in the description of the PR!

@Flyingmana
Copy link
Contributor

Flyingmana commented Jan 9, 2023

the description wants to revert it because of a formality.

but the change you want to revert has a proven benefit.
There is no sign of a negative impact this new event could have.
Its not conflicting with any of the possible alternatives.
Its something which has already proven to be usefull in a production system.
There was no effort to work on any alternative solution from someone else over the months.

My goal is to get changes in, which are an improvement, which are usefull, which do not cause harm.

❌ Iam clearly against merging this revert

@sreichel
Copy link
Contributor

sreichel commented Jan 9, 2023

My goal is to get changes in ...

There are plenty of open PRs that needs reviews/approvals. Why not reviewing them first? You picked one, unset draft status, ignored comments and merged it on your own (something nobody else do - or can do).

Why do you bypass default workflow?

Is your opinion more important than everyone else's? Three members say it should not had been merged yet ...

Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

PR #1490 was for a very particular use case of setting the validation rules (using class names) which are configurable in backend (by custom code). Instead of customization just on frontend_class, I suggested a general case which can be achieved by dispatching the event in setAttribute(). This allows customization on other properties such as source_model. Unfortunately, there was no reply from @woutersamaey. As he mentioned that it was still WIP, we do not know what happen to the issue. We do not know if it's in production.

How do we proceed in this case? I think the PR should be closed, or should not be merged.

If the event must be dispatched in the getClass() method, then I also think that it's unnecessary to use ArrayObject. The event can be dispatched earlier without the need to pass the $out param:
image
The observer can setFrontendClass() to insert additional class names, which will be picked up in the next line above.

For the above reasons, I vote to revert it.

@sreichel sreichel merged commit ccbe6b8 into 20.0 Jan 9, 2023
@sreichel sreichel deleted the revert-1490-patch-3 branch January 9, 2023 04:24
@woutersamaey
Copy link
Contributor

So much time and energy you're all wasting. It saddens me.

@sreichel
Copy link
Contributor

sreichel commented Jan 9, 2023

It saddens me.

Me too. Do you have time to review @kiatng suggestions (in your PR)?

@fballiano
Copy link
Contributor Author

So much time and energy you're all wasting. It saddens me.

In any type of community we discuss and argue for the people and things that we care about and any type of community hold the members accountable for their actions, not in a bad way, but to grow all together.

@fballiano
Copy link
Contributor Author

the description wants to revert it because of a formality.

man, you're literally the formality guy, whenever I want to change something you're answer is "make an RFC" and now you're arguing that I'm doing it out of a formality when I explained (here and in the original PR) that there were parts of that PR that I didn't agree with?

when you start a community effort and you're enforcing everybody to take the "high road" then you loose the possibility to "solely decide things" (cit.). Sorry, standards work for everybody and it's not ok that you want to have different standards when you want to decide something versus when other people want to decide something.

but the change you want to revert has a proven benefit.

that's not the point, the point is that it was not finished.
I for one don't understand why should we change an array to an arrayobject, nobody even care answering my question. and I wasn't the only one saying that.
you're the first one who doesn't want half baked solutions (rightfully) yet in this case you're acting totally the opposite.

There was no effort to work on any alternative solution from someone else over the months.

false, there were questions that were not answered. Not everybody has to fix another person's PR when the OP is not responsive. I for sure didn't see you helping coding other people's PRs, instead I quite see you coming and block everything with a "change request".

@Flyingmana
Copy link
Contributor

Note: the "you" is meant in plural, everyone who supported the revert, but also everyone who clashed with me on topics in the last 4 years.

So lets see, Iam part of the topic here, so we have now a place where its not offtopic. Thats good.

Some words in advance, because some of you like to finger point on me for not doing enough. Even in my lowest times, I still put 5-10 hours into OpenMage. Thats not much, but it is still above the average contributor who only checks in every few months.
And I take care of a lot of areas you dont see, or you dont even think of looking at.
When ever there are complaints, I try to do something constructive out of it, like opening an Issue or now a discussion space, or drafting an RFC, or drafting some resources internal and external.
But more about this later, maybe.

So by which standards do I act.
In context of something else 2 Days ago I already started to put them indirectly down as goals here: https://github.com/OpenMage/organizational/blob/d76cad9068d072bd9ca31bf156cb7514aade4801/Goals.md
Had to add the Stability part now, as I somehow missed this one, so the updated list: https://github.com/OpenMage/organizational/blob/6f4174e95b6d408adc8688f7f8893ab263dd8264/Goals.md

Stability is an important topic for me. Most times when I blocked a PR, it was falling back to this Goal.

  • Because it did introduce a big Backwards Incompatibility
    • or a smaller one into the version, which should not have them
  • it removed something some of our Users are still using and needing
  • it posed a high risk of introducing a bug (or actually did)
  • it was likely to cause performance issues
  • it did change multiple things add once (which )
    • like not only adding a Blog Post, but also changing existing ones
    • and also changing the formate of something in other ones

But stability for an OpenSource project also means having a clear direction and set of standards by which decisions are met. A project, where a small set of people can turn around the direction completely is not something stable, or sustainable.
So I now for 3 years advocate for the RFCs, because I want the project to have the stability it needs to survive longterm.
Why is this? Because most people who are deciding for or against OpenMage do a cost and a risk analysis. A project with a high velocity has also the potential for higher cost. This is still manageable. But a project with a high volatility in its direction is going to be a high risk.

Lets take the handling of Backwards Incompability and supported timeframes as an example. Its the topic which is most important for me, because its the most visible and relevant one for users. But also because its the one where I get for far over 3 years most of the complaints for how we, or rather I handle them. Back then the compromiss was people could put them into v20, and we do a long term decision as an RFC.
Now, people continued to complain, and somehow manage for 3 years straight and repeatedly still to not know that BC breaks (and also new Features) belong into the v20 branch. Then arguing next, that there were merged so many already so its fine for them, too.
3 years of complaining, and look how many of them actively contributed to get this actually solved via an RFC.

Really, 3 years of complaining, putting so much energy and time into arguments why BC is not relevant here, its not needed there, its just annoying at this place.
Really, it would have taken only 2 of you to bring the RFC forward, and the whole topic would have been done in less then a Quarter. Instead you prefer to complain for 12 times as much.
With some of the options you could have already retired all the BC stuff 2 years ago.
Even the most conservative alternative sees an end of support in latest 2,5 years.

3 years, and you were not even able to find 2 people who support you with making a reality out of what you want. Really, I did more for this things you want, then all of you together.

You want to have roles on "turn" basis? With elections and meetings?
The 3 meetings I did had in total 1 antandee, and I still got no suggestion for a better timeslot.
Elections? Great, but maybe first get a picture of what roles there are to fill. But no worry, guess who got you covered? https://github.com/OpenMage/organizational/blob/main/Roles_and_Repsonsibilities.md
But honestly, before anyone thinks about elections, maybe proof first that any of the candidates is able to do more then just coding. Like working together with someone to define new rules and processes for the Project instead of letting someone else do this for you. Think about something, which is creating something new, and not only about removing existing things because you feel restricted or overhelmed by it.

So if I annoy anyone with processes and creating an RFC, then also to prepare them to become a maintainer who acts in the longterm benefit of the project.
When I skip merge requirements, then to save time for others, and because Iam to 100% confident about the change and Iam also available to fix things in case I missed or broke something.

There was no effort to work on any alternative solution from someone else over the months.

false, there were questions that were not answered. Not everybody has to fix another person's PR when the OP is not responsive. I for sure didn't see you helping coding other people's PRs, instead I quite see you coming and block everything with a "change request".

yeah, you say nobody and that you didnt see me do this. I say Iam fixing other peoples PRs now for around 6 years in OpenMage. If there is the decision to either Fix or Close a PR from an unresponsive author, and if the PR is usefull, then I expect from a good maintainer to fix it.
Changing this to the expectation "the maintainers have to fix my errors", like it happened now recently. Then yes, I will fix it too, but its in my priority list sorted according the usefullness for the upperwards named Goals, and the amount of effort it is.
In this case here, the effort was low to fix the codestyle issues and the merge conflict, so I did.
And the Usefullness was given. And it did not stand in conflict with any other suggested solution. Like, you even can have both. There was not even a naming conflict.

So what actual Reasons are there to revert it?

  • I guess no trust in my ability to judge the impact of the change.
  • Putting formalities over user impact. (because there were no negative downsides on users)
    And yes, Iam still naming this, because you put a precedence by actively doing a revert PR and additionally dismissing my clearly named block of it.
  • starting off a confrontation, as you continued despite my strong opposing to it, so you had to expect this will happen.

So tell me, what kind of possible outcome do you see out of this scenario. How do you intend to turn it into something constructive. Who of you is going to take the first step, taking the lead into bringing the Project forward now.

@sreichel
Copy link
Contributor

Just to answer this for now ...

So what actual Reasons are there to revert it?

  • I guess no trust in my ability to judge the impact of the change.

  • Putting formalities over user impact. (because there were no negative downsides on users)
    And yes, Iam still naming this, because you put a precedence by actively doing a revert PR and additionally dismissing my clearly named block of it.

  • starting off a confrontation, as you continued despite my strong opposing to it, so you had to expect this will happen.

1st ... Cmon. No.
2nd ... The problem is you decided it on your own. Ignoring comments incl. suggestion made by @kiatng. Nobody else merges PRs w/o two approvals. This may be okay for urgent hotfixes, but not for regular PRs (adding a new feature here). I ask again, is your opinion more important than everyone else's? Was it really necessary to merge it? Why not review/approve another PR thats not in draft status?
3rd ... despite my strong opposing ... four participants stated that it should not be merged. Are you still ignoring their opposing?

To be honest, I would have expected you to show some understanding.

Maybe @woutersamaey can review this #1490 (comment) to move forward ...

@fballiano
Copy link
Contributor Author

Some words in advance, because some of you like to finger point on me for not doing enough.

nobody fingerpoints anybody for not doing enough, you got fingerpointed when you disappear for months and only appear when you're ovverriding other people's decision.

So I now for 3 years advocate for the RFCs

and I ended up agreeing, although the RFC process makes everything extremely slow. but I'm the only one who produced an accepted RPC and, if you believed in it so much, I would have expected more activity in the previous years, to put an order to the unwritten decisions that have been taken "solely by someone" before you decided everything should be an RFC.

Because most people who are deciding for or against OpenMage do a cost and a risk analysis.

but a project with no velocity has no developer interested in working in it, so you have to balance

Now, people continued to complain, and somehow manage for 3 years straight and repeatedly still to not know that BC breaks (and also new Features) belong into the v20 branch. Then arguing next, that there were merged so many already so its fine for them, too. 3 years of complaining, and look how many of them actively contributed to get this actually solved via an RFC.

yes, and since you are the major advocate for RFC, why don't we have one in 3 years? because for the branch rules I'm the one who wrote it out of the misery I was feeling for the endless discussions, and I'm new as a contributor.

You want to have roles on "turn" basis? With elections and meetings? The 3 meetings I did had in total 1 antandee

you're totally right on this point, I didn't have time to manage more, the "turn" thing was a "like to see for the future" not a "i'm complaining for 3 years" but I've explained the whole point in depth in the other discussion so I'm not repeating that long thing here.
for example, you manage the social network accounts for openmage, but I think those account should be opened and owned by the team, not only one person, and that not because we want to "take something from you" but because we want to keep openmage as safe and as alive as possible for the future.

yeah, you say nobody and that you didnt see me do this. I say Iam fixing other peoples PRs now for around 6 years in OpenMage. If there is the decision to either Fix or Close a PR from an unresponsive author, and if the PR is usefull, then I expect from a good maintainer to fix it. Changing this to the expectation "the maintainers have to fix my errors", like it happened now recently.

but not in the last year at least, that's what I saw. but anyway, again, nobody tells you fix anything, but if you come to a PR, block it over a detail when you could have changed it yourself in some minutes or at least talk about it... you've to understand that that's not cool.

So what actual Reasons are there to revert it?

they have been explained. is there an actual reason to merge a new feature that quickily when other maintainers were handiling it?

  • I guess no trust in my ability to judge the impact of the change.

false, we all know you're very technically skilled

So tell me, what kind of possible outcome do you see out of this scenario. How do you intend to turn it into something constructive. Who of you is going to take the first step, taking the lead into bringing the Project forward now.

we have PRs opened since years, it's no big deal if this one, which doesn't seem to me the most important one, stays open a bit more while we finish the discussion. but all maintainers are peers and you cannot rule on everybody else like this is solely your project. that is the main point of the whole argument.

@ADDISON74
Copy link
Contributor

@Flyingmana, no one will be able to erase what you have done for OpenMage and once again I thank you for that. When the project stagnated for a long time, about a year ago you did an important step and by giving maintainer rights to more people, the results did not take long to appear.

I have to admit that the project moved very well based on @sreichel and @fballiano contributions. If we want to give shine to this project we must not lose such people. At this moment Sven is the most important contributor to the project and he will keep this crown for a long time to come.

That PR was not a priority but aimed at solving a particular situation desired by a developer. I have nothing against though. I don't think it will be used by 99% of those who use OM. In any case, a documentation should have been proposed too.

I have a rule, I do not approve PRs where I have doubts, where contradictory discussions are necessary, where I am beyond the advanced level of modification that requires detailed analysis. I made such a mistake last year, I admitted my fault and I was more cautious. We are human, we cannot evaluate everything that happens in such a complex framework as OpenMage, we often rely on feedback, on tests. We have PRs that have been unanalyzed for years, if one will mold over a period of time this is the reality, it should not be approved just to close it and tick off an objective. I recommended that all old PRs and where the authors no longer respond or do not want to participate in the discussions should be put in Draft.

In terms of BC, branches. Last year a sorting of all the PRs was done, then the branches where each goes were also established. If an issue was created in the LTS branch it was solved, I think this is more important than having long discussions without concrete actions.

Some rules are needed to prevent issues, but I would like to be efficient and not to reduce everything to an RFC. Web_Notification is an essential tool at the moment and I don't have rights like in magento-lts, I think 3 green approvals would be enough. I would like to use this tool to find out the opinion of others who use OpenMage, I would like to get feedback, to stay in touch with users. We won't get it with just a discussion created here. Please note only 64 people are getting notifications about what is happening in the repository, down by almost 20.

Some public statement were said that I do not agree with. We are a team based on a common goal and no one should be able to use a shortcut like "God Mode". Some posts must be deleted from the repository as soon as possible because they affect the image of the project.

@sreichel
Copy link
Contributor

But honestly, before anyone thinks about elections, maybe proof first that any of the candidates is able to do more then just coding.

Maybe everyone writes what role they would assign themselves. Do you start?

@sreichel
Copy link
Contributor

And I take care of a lot of areas you dont see, or you dont even think of looking at.

Please enlighten us.

You have time to write novels, but ... what else?

Really, I did more for this things you want, then all of you together.

You don't even manage to follow a discussion you started (as here) ...

I'd ask to all active contributers/reviewers/maintainers to step back for one single month ...

@fballiano
@luigifab
@elidrissidev
@kiatng
@ADDISON74
@justinbeaty

... I am curious how much PRs/Issues will be resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Eav Relates to Mage_Eav
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants