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

Update dot option definitions to latest (2015) #89

Closed
wants to merge 5 commits into from

Conversation

r0ckarong
Copy link
Contributor

@r0ckarong r0ckarong commented Mar 20, 2023

The rdot.rb file had outdated definitions for the available options. I've updated this to best of my knowledge. This addresses a comment from #61

discussion #61 (comment)

Definitions taken from the linked 2015 version: https://www.graphviz.org/pdf/dotguide.pdf

@monora
Copy link
Owner

monora commented Mar 22, 2023

Thank you very much for your effort to add the "new" dot options! I hope you missed none ;-)
After merging #88 could you rebase this branch on top of it and add one test for one of the new options?

@r0ckarong
Copy link
Contributor Author

After merging #88 could you rebase this branch on top of it and add one test for one of the new options?

Of course.

@r0ckarong
Copy link
Contributor Author

Rebased and added new test. Passing fine.

@monora monora mentioned this pull request Mar 23, 2023
@monora
Copy link
Owner

monora commented Mar 23, 2023

Ups, did not see your rebase. Please have a look at master which now contains two commits containing your changes from #88. Could you please rebase your branch (attributes2015) onto origin/master.

@r0ckarong
Copy link
Contributor Author

I cherry-picked the relevant commits on a fresh copy of current master. Tests passing. I think we're good.

@r0ckarong r0ckarong force-pushed the attributes2015 branch 2 times, most recently from 99c3c83 to a18d9e9 Compare March 24, 2023 09:08
@monora
Copy link
Owner

monora commented Mar 24, 2023

Do you think it is easy to have a test, which validates the output of write_to_graphic_file? Which tests one or two of the new attributes. That is define a fixture png file with a simple Graph with one edge and compare the output in the test.

@r0ckarong
Copy link
Contributor Author

r0ckarong commented Mar 24, 2023

Do you think it is easy to have a test, which validates the output of write_to_graphic_file? Which tests one or two of the new attributes. That is define a fixture png file with a simple Graph with one edge and compare the output in the test.

I'm actually already working on a test that validates all node and edge options. I was planning to add this as another pull request because I will need some more time to define the proper examples that show the actual results of the options.

node-opts-graph

However, I currently wouldn't know how to compare the graphical output to some test. I am using the comparison and just putting out an image file as well. Maybe I can use the resulting SVG to compare against and see if the actual SVG tags are correct. If you know how to compare a fixture png to something the test produces I'm happy to learn how to do this.

@monora
Copy link
Owner

monora commented Mar 24, 2023

Maybe I can use the resulting SVG to compare against and see if the actual SVG tags are correct. If you know how to compare a fixture png to something the test produces I'm happy to learn how to do this.

Since I switched to python for day to day work, I am not firm with ruby test libraries. pytest is really good defining fixtures. But a simple file diff could also work.

@r0ckarong
Copy link
Contributor Author

r0ckarong commented Mar 24, 2023 via email

@monora
Copy link
Owner

monora commented Mar 24, 2023

I have a problem using the new version. This simple example does not show the expected result:

  dg = RGL::DirectedAdjacencyGraph[1,2]
  dg.set_vertex_options(1, shape: 'tab', fontcolor: 'blue')
  dg.set_edge_options(1, 2, color: 'blue')
  dg.write_to_graphic_file

graph.dot:

digraph RGL__DirectedAdjacencyGraph {
    1 [
        fontsize = 8,
        label = 1
    ]

    2 [
        fontsize = 8,
        label = 2
    ]

    1 -> 2 [
        fontsize = 8
    ]
}

graph

What am I doing wrong? I think we must have more documentation in set_vertex_options,set_edge_options and write_to_graphic_file. Perhaps with simple examples.

@monora
Copy link
Owner

monora commented Mar 24, 2023

Do you have a pointer on how this would be done in Python?

About fixtures — pytest documentation
pytest-datadir · PyPI

This is what I would use in python

@r0ckarong
Copy link
Contributor Author

r0ckarong commented Mar 24, 2023

What am I doing wrong?

You need to set up the options hashes edge_options and vertex_options with
the procs like in the _complicated_options test. This is the application
side configuration for which attributes should be parsed.

dg = RGL::DirectedAdjacencyGraph[1,2]
dg.set_vertex_options(1, shape: 'tab', fontcolor: 'blue')
dg.set_edge_options(1, 2, color: 'blue')

get_vertex_setting = proc { |v| dg.vertex_options[v] }
get_edge_setting = proc { |u, v| dg.edge_options[dg.edge_class.new(u, v)] }

vertex_options = {
'shape' => get_vertex_setting,
'fontcolor' => get_vertex_setting,
}

edge_options = {
'color' => get_edge_setting,
}

dot_options = { 'edge' => edge_options, 'vertex' => vertex_options }.to_s

dg.write_to_graphic_file(dot_options)

Maybe we should just provide this whole hash populated with all values through dot.rb and the user just has to worry about which parameters to pass to the methods. Let me think about how this can handle defaults that should be applied to everything.

I liked that with the current approach you have a specific list of options that you want to use and can override with defaults for all edges/nodes if you want. This is also hard coded for a couple of things like name in dot.rb but the user can't change that without modifying the library only overwrite the values.

The question would be if it's acceptable to have the method just listen to any of the available options and if the user wants to have a default value assigned via the methods they must make sure to implement calling the method with the parameter each time. I'm not very sure either way. I like the current way but it needs more explanation.

@r0ckarong
Copy link
Contributor Author

This is proving harder than I thought. There is something wrong (or I don't get it) between the edge options assignemnt and the way dot reads it. I'm going to dig more.

@r0ckarong
Copy link
Contributor Author

r0ckarong commented Apr 3, 2023

@monora

The convenience thing for the settings is separate from this PR. Could we move forward with updating the dot option definitions. The provided test already covers the new options.

Once I have more results for the exhaustive tests or the convenience improvement for the settings methods I'd add new PRs.

@monora
Copy link
Owner

monora commented Apr 3, 2023

The convenience thing for the settings is separate from this PR. Could we move forward with updating the dot option definitions. The provided test already covers the new options.

OK.

Once I have more results for the exhaustive tests or the convenience improvement for the settings methods I'd add new PRs.

Can you file an issue or PR which describes the problem to be solved, so that we can refer to it in the release notes later.

Thereafter we can merge this PR.

@r0ckarong
Copy link
Contributor Author

Can you file an issue or PR which describes the problem to be solved, so that we can refer to it in the release notes later.

I have opened #92 and filed #91 [WIP].

I'm currently blocked with other work but I'll get back to this.

Align comments for graph options
Fix keyword for dir in edge config and assertion

Fix name for comment quoting tests

Fix name for comment quoting tests

Fix test comment method names
@r0ckarong
Copy link
Contributor Author

Rebased this to #94, this should give us a direct line to have the convenient options handling and all the new attributes.

@r0ckarong r0ckarong closed this Apr 3, 2023
@r0ckarong r0ckarong deleted the attributes2015 branch April 12, 2023 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants