Graph format: Refactoring and enhancing

[gesinn-it-gea]

This PR is made in reference to: #

This PR addresses or contains:

• refactored, added SRFGraphNode class
• added handling for additional labels (label1 - label3)
• added record/Mrecord type shapes
• added arrowhead parameter
• added support for Display Title

This PR includes:

• Update of RELEASE-NOTES.md
• Tests (unit/integration)
• CI build passed

[gesinn-it-gea]

... I'll add a screenhot soon to show, how this new label parameters can be used.

[gesinn-it-gea]

Example Query:

{{#ask:[[Category:Team]][[Part Of Team::+]]
  |?Part Of Team
  |?Display Title=label1
  |?Casted Leadership Role=label2
  |?Casted=label3
  |format=graph
  |arrowhead=none
  |nodeshape=Mrecord
  |graphsize=20,20
  |graphcolor=no
  |graphlink=no
  |graphlabel=no
  |graphlegend=no
  |arrowdirection=BT
}}

[gesinn-it-gea]

In the top box, only label1 is used, in bottom left box, all three labels are used, in bottom right box, label1 and label3 are used (used means, that the query had results)

[gesinn-it-gea]

“This PR has received funding from the European Union’s Horizon 2020 research and innovation programme under grant agreement No 741782”.

[mwjames]

I'll be doing only a quick review given that my last comments on SFS went into thin air and I'm pressed with time during my support window.

This PR has received funding from the European Union’s Horizon 2020 research and innovation programme under grant agreement No 741782

Each PR should note the grant agreement so they can be easily distinguishable (given that those grants are created from tax payers money).

Here some initial comments without looking too much at the code itself:

• One class per file (see SRFGraphNode)
• Adding a new class -> requires a unit test
• SRF is namespaced, so newly or refactored classes should carry this characteristic
• PHP class properties don't require a prefix m_ (see private $m_id;)
• Ensure explicit visibility ( function __construct( $id ) {)
• Why does the parameter name change from srf-paramdesc-graphlegend to srf_paramdesc_graphlegend? (In general we want a hyphen as separator, not an underscore)
• Why do we change from the [ ... ] to array( ... ) as array declaration?

[gesinn-it-gea]

@mwjames uups, obviously something went completely wrong with this PR. Seems that I accidentally merged a wrong branch of the code from our internal gitlab. Also my PHPStorm code formatter has messed up something. I'll double check. Sorry.

[gesinn-it-gea]

@mwjames would you recommend moving the Graph format to an own directory (graph)? Currently it's sharing the graphviz directory with the Process format?

[gesinn-it-gea]

@SchmidDev please do not update this PR with temp. stuff. @mwjames (and others) please ignore latest commits. The whole thing is a complete mess now.

[mwjames]

would you recommend moving the Graph format to an own directory (graph)

Yes.

CI failures have been fixed with 9769c97.

[mwjames]

@gesinn-it Are we going to continue this one, would be a shame if steam is running out here!

By the way, I saw something peculiar, the naming of getLabel1, getLabel2, and getLabel3. Either use a more descriptive name or simply use getLabel(...) where it takes an argument and can generate any number of (maybe something like 4 or 5 in future) labels. Having 1,2,3 as name doesn't really appeal to me.

[gesinn-it-gea]

@mwjames yes, we are definitely continue this one. I'm traveling the next two weeks (including SMWCon). I hope I can finish latest during SMWCon...

[mwjames]

#314 can guide you on the principles of how a refactoring effort can look like.

[gesinn-it-gea]

@mwjames thanks for the hint with #314. I'll have a look at it. I continued this one but still a few things to clean up. Hope to have it ready before next weekend.

[gesinn-it-gea]

GraphNode

I couldn't see any GraphNode unit test (use the standard inheritance
PHPUnit_Framework_TestCase)

Each class is tested on its own which means GraphNode =>
GraphNodeTest (avoid mingling otherwise it becomes some form of an
integration test).

I'll create separate GraphNodeTest.php file

[gesinn-it-gea]

GraphResultPrinter

There are a lot of hard-coded strings like $graphInput .= "[label=\"{" . $label;, is it possible to isolated this in a more
formalized and intelligent formatter class that only deals with the
formatting and once done outputs the final string?

Good idea, I'll create a GraphFormatter class + GraphFormatterTest...

[s7eph4n]

Would it make sense to change the PSR-4 pointer in composer.json to the /formats/ directory instead of creating a /src/ directory for one format?

[mwjames]

Would it make sense to change the PSR-4 pointer in `composer.json` to the /formats/ directory instead of creating a /src/ directory for one format?

Refactored formats should go into `/src` as what happened in #314. This is to get some uniformity and have somewhat of a task list about the once that are tested and those that contain "old" code. There is no rush to make the switch and only the once that are truly refactored should be moved to`/src`. Cheers

…

On 10/12/17, Stephan Gambke ***@***.***> wrote: Would it make sense to change the PSR-4 pointer in `composer.json` to the /formats/ directory instead of creating a /src/ directory for one format? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #297 (comment)

[s7eph4n]

One of my biggest problems when looking for something in the SMW code is that I never know, in which folder to start. Given the structural changes in SMW the "clean slate" approach makes sense, but I think SRF is already structured well enough and any new folder structure will look pretty much the same as the old one.

I would also challenge the other two points. Arranging the project structure to have a tasklist is somewhat similar to putting ToDos and FixMes in the code. If we want a task list we should use a Github issue/project/whatever.

And test coverage or 'refactoring done' as a criterion to move formats to the new folder will probably not work either. I am working on the Filtered format right now. Done some refactoring, but it is still messy. Move over or not? Same goes probably for a number of other formats.

Finally with SRF we are in a situation, where usually a format is worked on when somebody has an issue with it. Given this approach we will end up having the src and formats folders side by side forever because some zombie formats will never get cleaned up.

So, I would suggest

1. Moving all formats to src (or what about src/Formats?)
2. Creating a Rework project on Github
3. If possible removing the unused, unmaintained formats

[mwjames]

Moving the discussion about refactoring in general to #321.

[mwjames]

commented 11 days ago

@gesinn-it Any update?

[mwjames]

This PR is open since Aug 31, and we still have some unresolved issues. Do we expect an update in near future?

[gesinn-it-gea]

@mwjames: I'm very busy with projects at the moment. I will definitely need some days to do the changes you have suggested. No worries that this PR dies, because I need the changes to the graph format myself ;-)

[mwjames]

If this ever gets completed, please see #365.

[mwjames]

@RacoonDev
manage invalid offset of label
c0afa5d

Please make sure you use the master (see section "Conflicting files") as base, furthermore please DO NOT change the composer to something like:

@RacoonDev
change semantic mediawiki version because of dependency problems
44fc73c

Master works against master and against a restricted branch like "~2.5.3".

[mwjames]

@kghbln @hexmode @krabina @gesinn-it @RacoonDev This PR has been stuck here for nearly two years and I'm wondering whether we can resurrect this change and finalize it, otherwise we need to close it.

Also, as indicate above I expect that people committed to a PR do finalize it in an appropriate amount of time. Please consider this as a topic for discussion among yourself for the next SMWCon about how to improve contributions and what is needed to make them an actually contribution to the overall SMW community.

Note

An epic task of refactoring should be split into smaller pieces making room for early adaptions without necessarily impairing existing functionality.

[mwjames]

As for "added support for Display Title" see #496 which was merged prior given the unknown state of this PR.

[gesinn-it-gea]

@mwjames this PR really got stuck. Development progressed somewhat bumpy between branching 3.0 and general refactoring. In the meanwhile, the code base of SRF has evolved. I think we should consider closing this PR and re-implement on current code base... The gesinn.it team could do this job during the next weeks.

[mwjames]

re-implement on current code base... The gesinn.it team could do this job during the next weeks.

I would appreciate that and maybe doing it in smaller iterative steps might help to get this done where you use the current PR as blueprint for the expected outcome.

…

On 7/22/19, gesinn.it ***@***.***> wrote: @mwjames this PR really got stuck. Development progressed somewhat bumpy between branching 3.0 and general refactoring. In the meanwhile, the code base of SRF has evolved. I think we should consider closing this PR and re-implement on current code base... The gesinn.it team could do this job during the next weeks. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #297 (comment)

[SbstnS]

@JeroenDeDauw could you please close this PR.
Thanks!

[kghbln]

could you please close this PR.

Done