Use TextWriter instead of StreamWriter where applicable #322
Conversation
This enables a more efficient implementation of the `string GiGraph.Dot.Extensions.DotGraphExtension.Build(…)` method (using a `StringWriter`) as it doesn't have to perform any encoding conversion. Also, this potentially enables better interoperability with the .NET ecosystem where methods may expect an `Action<TextWriter>` or `Func<TextWriter, Task>` delegate. Note that this change was carefully implemented to observe both source and binary backward compatibility.
|
Thanks a lot for the input, I appreciate it! :) I'll just merge the code as is, and adjust the things I mentioned in the comments to not bother you with those minor changes. |
| @@ -32,21 +33,17 @@ public static class DotGraphExtension | |||
| /// The syntax rules to use. | |||
| /// </param> | |||
| /// <param name="encoding"> | |||
| /// The encoding to use for the output text. | |||
| /// Unused. Kept for backward compatibility. | |||
| /// </param> | |||
| public static string Build(this DotGraph graph, DotFormattingOptions formattingOptions = null, | |||
| DotSyntaxOptions syntaxOptions = null, DotSyntaxRules syntaxRules = null, Encoding encoding = null) | |||
There was a problem hiding this comment.
The encoding may now be removed since it is not used any more here.
There was a problem hiding this comment.
Well, removing a parameter from a public method is both a source and binary breaking change. As a consumer of libraries, I like when they follow Semantic Versioning and don't introduce breaking changes in minor version updates. That's why I intentionally left the parameter untouched.
There was a problem hiding this comment.
Makes sense. However, I'm not actively developing the package now. I think I got to a point where I implemented all that I planned to implement, and don't currently have any more new ideas to include, unless there appear requests or suggestions like yours ;). So far I haven't used that versioning approach (but maybe it was wrong of me). I changed versions based on the amount of changes or new functionalities introduced, without paying much attention to backward compatibility. Thanks for bringing it up though, I'll consider this approach in the future!
| using var outputWriter = encoding is not null | ||
| ? new StreamWriter(output, encoding) | ||
| : new StreamWriter(output); | ||
| var initialCapacity = (graph.Nodes.Count + graph.Edges.Count) * 16; |
There was a problem hiding this comment.
It is actually more complex than that, and I'd probably not specify the initial capacity as it is hard to estimate it. The graph would have to be traversed recursively to know how many nodes and edges there actually are. The thing is that the graph may be composed of subgraphs and clusters, but also there are 'subsections', being parts of the output, each of which may as well contain edges, nodes, subgraphs and clusters.
There was a problem hiding this comment.
I'm aware that's it's a very very rough estimate. I thought giving an initial capacity would be better than 0 which will automatically grow at the first write. But I must admit I haven't benchmarked anything and that not specifying any initial capacity might be totally fine.
| @@ -97,8 +119,36 @@ public static class DotGraphExtension | |||
| /// <param name="graphGeneratorBuilder"> | |||
| /// The graph generator builder to use in order to get the graph builder to generate the DOT output with. | |||
| /// </param> | |||
| [Obsolete("Please use the equivalent Build method taking a TextWriter instead of a StreamWriter")] | |||
There was a problem hiding this comment.
So far I never cared about backward compatibility. It would cause me a lot of troubles trying to give the library the current shape. I would just remove the obsolete code ;). Especially that I believe that these pieces of code won't affect anyone.
There was a problem hiding this comment.
As a first-time user of GiGraph I don't personally care. 😁
I love it when maintainers merge my pull requests and then adjust themselves for minor changes, it avoids a lot of back and forth. 👍 Also, thanks for your awesome library and for your excellent README ! I wish all libraries were so well documented. 🤩 I used GiGraph in this micro project and it was a breeze: https://github.com/0xced/RidGraph |
|
Thanks for this comment. It's so nice to get such feedback, especially that I spent a lot of time to give the library and the readme the current shape 😄. Hope you'll find it useful. Cheers! I'll have a look at your repo out of curiosity, thank for posting the link! It's good for me to know the scenarios people use the library. It gives me feedback about what may make sense to improve or change. |
This enables a more efficient implementation of the
string GiGraph.Dot.Extensions.DotGraphExtension.Build(…)method (using aStringWriter) as it doesn't have to perform any encoding conversion.Also, this potentially enables better interoperability with the .NET ecosystem where methods may expect an
Action<TextWriter>orFunc<TextWriter, Task>delegate.Note that this change was carefully implemented to observe both source and binary backward compatibility.