Alternative solution for #6

[manulera]

Hello @kltm this is an alternative solution to avoid code duplication. It also handles some errors (self-referencing relationships should not be allowed + returns error if waling_fun returns the same id passed as object)

https://github.com/manulera/bbop-graph/blob/079aa824970320768465d26c28e0993cccd314f9/lib/graph.js#L1264-L1278

I tried to write a test, but I can't get the gulp scripts to run locally. If you add instructions of installation (node version where it works, etc.), I can add them. Otherwise, here is a script to test that it does what it should, if you create it in the root dir:

const library = require('./lib/graph')

async function main() {

    const go_resp = await fetch(
        "http://current.geneontology.org/ontology/go.json"
    );
    const go_json = await go_resp.json()
    const g = new library.graph();
    g.load_base_json(go_json.graphs[0])

    console.log('total nodes:',g.all_nodes().length)

    var anc = g.get_ancestor_subgraph('http://purl.obolibrary.org/obo/GO_0006366');
    console.log('number of ancestors:',anc.all_nodes().length);

    var des = g.get_descendent_subgraph('http://purl.obolibrary.org/obo/GO_0006366');
    console.log('number of descendents:',des.all_nodes().length);

    const walker_right = g.walker(g.get_child_edges, 'http://purl.obolibrary.org/obo/GO_0006366', false);
    console.log('should match number of ancestors:', walker_right[0].length)

    // Should throw an error
    g.walker(g.get_child_edges, 'http://purl.obolibrary.org/obo/GO_0006366', true);
}

main()

[manulera]

For being able to run the code in other machines, maybe committing your package-lock.json might be enough? Not sure about that.

[kltm]

@manulera Apologies about the pause here. While I've been able to run tests manually using the CLI, I too have been having issues with automation. I'm going to give it one more go and then I'll start taking some fix just based on manually running tests.

[kltm]

@manulera Thank you for this work. I'm unsure if I feel that code duplication is a big deal in this case--I think it makes it a bit more clear what's going on without getting wrapped up in subject/object. As well, I'd rather not change the function signature or default behavior for a public function in the prototype. However, if possible, I would like to include your additional checks and tests in https://github.com/berkeleybop/bbop-graph/pull/8

[kltm]

I think that checking for object == subject isn't too useful here as the walker (IIRC) already deals with cycles by coloring and the start node is already included in the graph. If not, that might be a bug and we'd need to check the tests.

[manulera]

I think that checking for object == subject isn't too useful here as the walker (IIRC

True!

I will use the new version of the library instead of my patch, but I still think it is best practice to avoid code duplication when possible, also because if someone wants to see the difference between those two functions, they have to find the one-line difference. Adding an optional forth argument would keep the function looking more similar to its current implementation and avoid code duplication. Back-compatibility is not really an issue, since the function names have changed. And as I mentioned in the other comment:

If you absolutely want to have the walker_up and walker_down functions, those two functions could call walker with the true / false argument.

This would keep the same argument structure for walker_up and walker_down as now, without duplicating code.