Jump to content

Under review... patches. Ask to Devs


Recommended Posts

i really see the problem, actually there are 4 pages of patches in the 'under review' section... but the title of this section is wrong, it's a 'needs review' section, not a 'under review'... i take now as an example my own patch, [patch]Don't delete chars finally, the last comment from a dev (i just looked at 'green names' now ^^) was at the 11/30/08, since then no one from the dev-team wrote any comment in there... so i have no idea if there is something to change...

and this is only one example, there are much other patches which don't get any review, and i think this isn't good, no one wanna submit a patch if he allready knows that this patch won't get reviewed... for me a little comment as "will be reviewed till 04/01/09" would allready be enought, so i would know that there's going something on

Link to comment
Share on other sites

i really see the problem, actually there are 4 pages of patches in the 'under review' section... but the title of this section is wrong, it's a 'needs review' section, not a 'under review'... i take now as an example my own patch, [patch]Don't delete chars finally, the last comment from a dev (i just looked at 'green names' now ^^) was at the 11/30/08, since then no one from the dev-team wrote any comment in there... so i have no idea if there is something to change...

and this is only one example, there are much other patches which don't get any review, and i think this isn't good, no one wanna submit a patch if he allready knows that this patch won't get reviewed... for me a little comment as "will be reviewed till 04/01/09" would allready be enought, so i would know that there's going something on

No no, there are actually 11 pages ~ 230 patches, but about 50% are not needed, outdated etc.. those 4 pages are only from last month..

Link to comment
Share on other sites

  • 39 years later...

I want to ask if Devs can give some advice or clues, or even comments to patches posted in Under review section. I know that some Devs are focused on other important things( Dislord on maps optimisation, Tom_Rus on 3.1, Triply on BG stuff) but there are some great and imo rdy for git patches without Dev comment. It can confuse posters a bit. Just want to ask if u can tell them what should they improve or correct. It will be very helpful. THX

Link to comment
Share on other sites

Developers of mangos core are just great, but the problem exists (time)

hope some more developers will be added to the team (I want thenecromancer back to rock) who can AT LEAST give advices on the approach to use.

Something like a filter Dev who will say to experienced Devs like Vladimir which patches are REALLY ready for review.

Link to comment
Share on other sites

Perhaps even users without some kind of "this is classified developer" sign can make suggestions there. Users that know the "basic rules of thumb", like that patch with something similar to "if (spellid == 1234 || spellid == 3241)" won't probably get accepted (if other way exists) and such.

Link to comment
Share on other sites

Guys, I just don't get it. There are tons of working patches in the under review section.. There are outdated too, and i know that nobody wants to fill mangos with a lots of crap. BUT there are working patches, confirmed working from a lots of testers, and as I see the commits, spell/talentfixes almost never get committed.

I'm not a coder, I dont exactly know how theese things getting done, but I surely know, that those patches are not there, to get outdated :\\

Link to comment
Share on other sites

BUT there are working patches, confirmed working from a lots of testers
The way I see it it's important that you test patches but from a patch review standpoint it's just a hint if it's at least worth considering, a minimum requirement. Nobody cares how many noobs say it works and ask for the patch to be added, in fact it's more annoying when they do. In part this discrepancy comes from the fact that users think in short term, they want features NAO!!, while we try to think more in the long term, how we can manage this huge pile of code while converging towards something perfect. A patch needs to be a lot of other things in order to be "correct" like efficient, elegant, extensible, concise, consistent, leak free etc etc etc. You cannot prove that a patch is "correct" by testing, only that it's incorrect. It can only be proven by actually providing a proof which is in the vast majority of cases not done so that delays the review as the developer has to do it himself and may result in him losing interest in the patch altogether .. And interest is the most important word here, this is all voluntary work and most people won't do things they don't like doing. Everyone has different interests and some people just pick random things to do while others have their own priority scale and will gravitate towards things they see as more important. The point is nomatter how important YOU think something is, you cannot force someone else to do it. You can reason why it would be good idea to do something but in the end it's entirely up to them. You can only measure someone by what they do and not what they don't do. Other things factor in like whether a certain developer knows a certain part of the code well or whether he remembers to look up older patches and bugs and so on but all of these lead to the same conclusion .. The more people the bigger the chance that someone will pick something up. Unfortunately there are few of us doing active work so things go slow .. we're open to suggestions on how to improve this but idiots who start their own projects don't help ..
Link to comment
Share on other sites

we're open to suggestions on how to improve this but idiots who start their own projects don't help ..

As I mentioned before in another topic, among the hardest to review patches, are the big SQL patches. Though they do not require any knowledge of the code. So maybe it is a good idea to have an SQL branch in the under review section, and some kind of SQL Devs to handle those patches. Or maybe leave more tables to be handled by DB, like spell_proc_event and spell_bonus_data.

Link to comment
Share on other sites

Though they do not require any knowledge of the code.
Are you sure? ;)

spell_tables is just helper data for code work. _Often_ SQL data useless without code support. And in anycase it required testing and debug code for check correctness, or maybe outdated code.

Wyk3d, your post break my strong personal possition ignore this and similar thread totally after past october. ;) But i still think that discuss this just useless and harm for team and for readers, for project. Anyway this not change our possition and not chnage way how each team member select what he will do next in project. For me like cry-threads just harm my fun from mangos development and harm my project work then. I think something similar applied to all readers in osme rate.

Link to comment
Share on other sites

For me like cry-threads just harm my fun from mangos development and harm my project work then. I think something similar applied to all readers in osme rate.

for me personally this here was not a cry-thread, but more a thread where i'm asking for informations. we're all here to help and improve this software and to learn more. but i'm interested in that, what the team does. i just wanna know what you're doing, why should i submit a patch when he never gets reviewed nor i get any comments? just to let there be a post in the 'under review' section which gets outdated after some months? then i can develop it for myself and don't share it, but that's not the goal of this project, the goal is, to share the code so that everyone can learn from the others (as far as i interpreted this project goals)

however, i'm still going to work on mangos and submit patches and hopping that they'll be in the repo once ;)

keep up your good work, vladimir!

Link to comment
Share on other sites

why should i submit a patch when he never gets reviewed nor i get any comments?

I think you submit patches for share with other ppl... with community. Yes, when patch added to offcial sources it solve many problems for author: now mangos team problems have related code up-to-date, fix found bugs, and etc. BUT... ppl can use patch and without adding to official sources.

i just wanna know what you're doing,
I for example (except cases when i write some big patch many days) don't know self what i will do next ;) Dependent from last read threads or bug report or some point writed at some team discussion. I not very orginized man...
however, i'm still going to work on mangos and submit patches and hopping that they'll be in the repo once
I glad to read this. Maybe we all have different views how project must developed, but we all work at project development and improving. :)
Link to comment
Share on other sites

From other side, to find patches in under review, use them, update them ... you can learn something with it and just don't download and run..

Anyway some comment, for example in arena thread case.. a lot of people didn't know that patch needs some rewrites and devs are working on it.. maybe some information in topic would be good. Or, we are planing to review this after some changes.. or sth.

Link to comment
Share on other sites

Someone who got the point of posts in this thread (and knows the MaNGOS code style) can write a small guide on how to write patches both from the formatting point of view (4-space indent, emacs bracket style) and from the code style and maintainability point (no direct spell IDs if possible, ...).

In fact, the French guy in the guidelines topic (https://getmangos.eu/community/showthread.php?t=272) is right, just extend http://wiki.github.com/mangos/mangos/codingstandards and put it somewhere here, on the forum.

Link to comment
Share on other sites

So maybe it is a good idea to have an SQL branch in the under review section, and some kind of SQL Devs to handle those patches.
You do need to know the spell system to work with those properly as Vlad said, but in general, having multiple categories for patches is not a bad idea I think. It would take some work to make good helpful categories but if done well it could help when someone wants to work on some subsystem in getting an overview of what patches have been written for that. We have a lot of devs on the team but most come and go. Getting a quick listing of patches in their field of specialty could let them know what others have worked on to avoid duplication and perhaps keeps some patches from being lost. This would also come with more maintenance issues though and the overall benefit may be small so it's unclear whether it's worth it ..
Link to comment
Share on other sites

You do need to know the spell system to work with those properly as Vlad said, but in general, having multiple categories for patches is not a bad idea I think. It would take some work to make good helpful categories but if done well it could help when someone wants to work on some subsystem in getting an overview of what patches have been written for that. We have a lot of devs on the team but most come and go. Getting a quick listing of patches in their field of specialty could let them know what others have worked on to avoid duplication and perhaps keeps some patches from being lost. This would also come with more maintenance issues though and the overall benefit may be small so it's unclear whether it's worth it ..

Yea..

Under review ...

- SQL Patches

- Packet structure, dbc, maps, vmaps, client stuff

- Core fixes, features (spell system, missing features..)

- Misc - small fixes .. like crash fixes etc .. there is a lot of one-two lines patches

??

Link to comment
Share on other sites

other idea (but near to the one from charlie2025):

- sql fixes

- network / client stuff

- dbc / maps / vmaps / similar things

- spell fixes (core, not db)

- crash fixes

- other fixes (small fixes, fixes which doesn't apply to any other topic above)

Hm i think if you create spell fixes category, you should create fixes also for pvp things, battleground stuff, etc.. and it would be a lot of categouries and would not be purposefull. I think it should be three / four max forums.

Link to comment
Share on other sites

i just remarked, that both, mangos and sd2 forum, are vBulletin. the sd2 forum takes use of the 'project tracker', which is, as far as i see, a really good thing, you can give a priority, you can select a category (for what it is, like 'gameobject'), etc.

maybe it would be a good idea to use this functionality in this forum here too?

Link to comment
Share on other sites

If you're gonna change the categories for patch submitting maybe it would be good to also create an own category for custom patches? In my opinion the forum would be more organized if all custom patches were gathered at one place instead of spread out in the core development section. Also, I don't think there should be too many different categories for spell fixes, crash fixes etc. I think one for SQL fixes and one for core fixes is enough (and maybe one for custom patches). Well.. that's just my opinion.

Link to comment
Share on other sites

If you're gonna change the categories for patch submitting maybe it would be good to also create an own category for custom patches? In my opinion the forum would be more organized if all custom patches were gathered at one place instead of spread out in the core development section. Also, I don't think there should be too many different categories for spell fixes, crash fixes etc. I think one for SQL fixes and one for core fixes is enough (and maybe one for custom patches). Well.. that's just my opinion.

I like the "small, one-line fixes (typos/etc.)" section :)

Link to comment
Share on other sites

Guest
This topic is now closed to further replies.
×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue. Privacy Policy Terms of Use