-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
Conversation
There was a problem hiding this 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
it is literally in the description of the PR!
the description wants to revert it because of a formality. but the change you want to revert has a proven benefit. 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 |
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 ... |
There was a problem hiding this 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:
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.
So much time and energy you're all wasting. It saddens me. |
Me too. Do you have time to review @kiatng suggestions (in your PR)? |
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. |
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.
that's not the point, the point is that it was not finished.
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". |
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. So by which standards do I act. Stability is an important topic for me. Most times when I blocked a PR, it was falling back to this Goal.
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. 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. 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. 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? 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.
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. So what actual Reasons are there to revert it?
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. |
Just to answer this for now ...
1st ... Cmon. No. To be honest, I would have expected you to show some understanding. Maybe @woutersamaey can review this #1490 (comment) to move forward ... |
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.
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.
but a project with no velocity has no developer interested in working in it, so you have to balance
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'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.
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.
they have been explained. is there an actual reason to merge a new feature that quickily when other maintainers were handiling it?
false, we all know you're very technically skilled
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. |
@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. |
Maybe everyone writes what role they would assign themselves. Do you start? |
Please enlighten us. You have time to write novels, but ... what else?
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 ... I am curious how much PRs/Issues will be resolved. |
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.