Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Fixed for wrong recursive profiles adding for custom profiles #122

Merged
merged 5 commits into from
May 29, 2015

Conversation

dlebedynskyi
Copy link
Contributor

suggested fix for
image

we have issues with expected behavior for custom profiles. see comment below.

@dlebedynskyi
Copy link
Contributor Author

There is a difference in expected behavior.
for custom profiles we have expected recursive list to be false while for some profiles like aqua regia we expect it to be true and override further.

This fix so far fixes issue above while with custom profiles while issue with regia remains. at lvl 25 we get not recursive list profession.

My changes are following - for custom profiles unless specified - set recursive to false.
For default profiles aka 'aqua regia' I changed logic a bit for code to

  • new profile overrides base.
  • if new profiles is missing next level task and is recursive set level +1 task to previous. This is to ensure that we do have priority of new over base.
  • if this is recursive and we still have nothing use base from previous level.

if somebody can make it sufficient simpler you are welcome. For now this one works for me.

@noonereally
Copy link
Collaborator

I am really not sure how we add profiles.
(Not specifically about this pull request but about how we addProfiles or how we want to add them).

How I see it the main issue is profile clutter, and long repetitive profiles.
There are two solutions for that specific problem, expend last level to empty level and override profiles to known base profile.

The order of those operations matter greatly, and as i see it, it should be expand and later override.
The expansion rule is simple
for (var i = 1; i < maxlevel <25> ; i++) if (!level[i]) level[i] = level[i-1]

Then it can override a base profile if needed.
This will allow an easy, simple and fast way to make full custom profiles that change the last levels but will work and all levels.
And profiles like gathering.
The expand will leave the first nulls as they are and new_profile = $.expand(true, {}, baseprofile, tempnewprofile) will override the nulls with the base profile.

I don't know if this is what called recursive?
I don't think there is a need for a special flag for this, it should always be like that.

There was another suggestion to add the former line to the current one (even if both not empty).
As I understand this is done to fill tasks that are relevant from previous levels or to create wider fallback.
Imo, the idea to add '+' was a good one for this, it will make clear and visible that this happens.

In any case, I think that all the profiles that come out of addProfile function must be complete with all levels and all flags so a later addProfile can use them as base.

I apologize if I wrote things that seems trivial, but I want to confirm that I understand correctly what it does (or should do).
If I'm wrong or there are other suggestion, as always suggestions are welcome.

noonereally added a commit that referenced this pull request May 29, 2015
Fixed for wrong recursive profiles adding for custom profiles
@noonereally noonereally merged commit 8c48f98 into Phr33d0m:master May 29, 2015
@dlebedynskyi
Copy link
Contributor Author

You suggestion is good. we had to much different ideas on how it should work. And extra got added with mass refine profiles. I think we can come to simpler solution. I like the idea with '+'.

as for now I tried my best to describe in clearest way what is going on now. I also not sure why we have some lines for expand.

@noonereally noonereally mentioned this pull request Jun 1, 2015
BigRedBot pushed a commit that referenced this pull request Apr 8, 2016
Fixed for wrong recursive profiles adding for custom profiles
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants