Closed WloBeb closed 9 years ago
@noonereally In my opinion addProfile() should be something like: addProfile (profession, newProfile, baseProfile) when baseProfile is null, undefined or {} clean newProfile should be added. if baseProfile isn't empty - concatenation. And it should be easy:
If you do not understand, all my comments here are related to @Phr33d0m, who has accepted all the changes from @dlebedynskyi (even new AddProfile() function which to me is too complicated and probably bugged), and rejected all mine. For me, it means that he has no time for this project and accepts / rejects changes depending on who introduced changes, even without looking at them. Hence my conclusion that I don't want participate in his branch. I'm thinking of Your branch, which has a lot of useful changes but I don't know where some of them lead ...
addProfile I think your concate/ expand code should be in it, so the "3. if one of them is recursive - set recursive flag" is not needed at all. There will be only base (default) profiles and all the rest will be added with the function (or even those) They will be expanded on the go, and the added profiles will be always complete with no missing parts. Yes this will make the addProfiles more complex, but, imo, with the right comments and explanation it will make the task logic much more 'linear', you take some task levels, expand them if needed, make profile and add it.
The Fork: As you probably saw I favor more options, if someone wants something he/she can activate it / disable it, or activate a flag. Like I modified your commit and added a settings option for both mining claims and leadership thing. Like here with the expand ( I see two good ways that the levels can be copies, so in my opinion the best way is to just have option for both, the implantation is negligible to add the flag. )
Honestly, I have my preferences and can be stubborn about them, and will modify stuff to adhere. But I try to be open to different views.
More specific:
@WloBeb addProfile does exactly what you described. It is not complicated and you should probably test before saying something really. It is really 0-7 of what you asked plus extra stuff you have not even thought off. So yeah at least use plunker to test stuff (especially when it is provided with link).
@noonereally I think you got something wrong :-). This is not my branch/repo. This is the official repo where everyone can (and should) contribute with stuff to get this better and better. I'm just a kind of 'mediator' to keep the code as clean as possible and other minor stuff like that, as well as not-that-bad-programmer ;-). People I think are capable of those things have direct write access to the repo so they can decide over stuff the same way I do.
@WloBeb:
You both implemented different ways of the same idea and which one is better and why should be discussed over politely and respectfully. I picked one up only by sight-reading the code, I might be wrong and a different approach might be neccessary. That's something we can see only through testing.
Thanks for being adults and understand.
@noonereally If you are interested I rewrote all stuff related to task definition:
EDIT: expansion may apply only to non-empty lists (like in your idea) or it can be removed.
The zax was a critical bug (imo) so i've commited when everyone agreed, butI donth think it's my place to decide for @Phr33d0m what to commit or not into his branch. It's a shame that everyone pulling into their way instead of taking a bit time and taking the best of every idea. I think that the best approach in this case will be to refactor the add profile function further, including the recursive expansion with the appropriate parameters. something like this for example:
So then:
Where concate add previous lines to current and expand will add previous lines only if current is empty. So: