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

[WIP] Rewrite ByPropertyIdArray #203

Closed
wants to merge 14 commits into from
Closed

Conversation

Benestar
Copy link
Contributor

This should replace ByPropertyIdArray. Just a draft for now, needs lots of testing to make sure it actually replicates the behaviour of the old version of the class. However, I'm already quite proud of using only half as much codes as the old implementation.

* @license GNU GPL v2+
* @author Bene* < [email protected] >
*/
class ByPropertyIdArrayNew {
Copy link
Contributor

Choose a reason for hiding this comment

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

\o/

@JeroenDeDauw
Copy link
Contributor

So you are going for a class that does the same thing? I'd rather see property id based indexing, grouping and move logic be kept separate. As per #22, having a StatementGroup object or so is probably a good way to move forward with that. You'd be able to move statements within the group, and be able to move the group in the list of groups.

@Benestar
Copy link
Contributor Author

Actually I want to keep ByPropertyIdGrouper as simple as possible and not add any move logic into it. Furthermore I'm not sure how we can do this in another class more cleanly.

One suggestion would be to pass two indices, one of the property group and one inside the group but this would require changes in the javascript ui.

@JeroenDeDauw
Copy link
Contributor

You could have the move logic for a statement within a group in StatementGroup and the logic for moving a group in a list of groups in StatementGroupList. And if a StatementGroupList contains instances of StatementGroup, and allows for only one group per property, then the rules of the domain are actually enforced properly.

@Benestar
Copy link
Contributor Author

Would this StatementGroupList then replace the current StatementList?

@Benestar Benestar force-pushed the bypropertyidarrayrewrite branch from bd1baf6 to ca248ee Compare September 24, 2014 19:58
@Benestar
Copy link
Contributor Author

To avoid confusion, this is going to replace ByPropertyIdArray before the WIP is removed from the topic.

@JeroenDeDauw
Copy link
Contributor

Would this StatementGroupList then replace the current StatementList?

In some places it will.

@JeroenDeDauw
Copy link
Contributor

Also note that moving logic was added to ReferenceList at some point

@Benestar Benestar force-pushed the bypropertyidarrayrewrite branch from 4ed0df7 to 5935c7d Compare October 9, 2014 18:20
@Benestar
Copy link
Contributor Author

@JeroenDeDauw @thiemowmde is it worth to work on this PR again or are there already alternatives to ByPropertyIdArray?

@JeroenDeDauw
Copy link
Contributor

There is the ByPropertyIdGrouper that you created... no further work has been done in this direction IIRC

@thiemowmde
Copy link
Contributor

I find this class scary, before and after the rewrite. I will not spend time on this.

@Benestar
Copy link
Contributor Author

Benestar commented May 7, 2015

New approach made in #479

@Benestar Benestar closed this May 7, 2015
@Benestar Benestar deleted the bypropertyidarrayrewrite branch May 7, 2015 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants