Implementing a simple graph api #14

thisiswhereitype · 2017-07-14T12:49:36Z

No description provided.

thisiswhereitype · 2017-07-14T16:26:52Z

I have made a mistake with git and this code actually implements two graphs and but only uses the older one which was intended to be removed. I will have a commit up soon rectifying this.

snowleopard · 2017-07-15T13:15:57Z

@thisiswhereitype No problem. Please let me know when the PR is ready for review.

…new graph. VHDL section has been disabled but I commit again before the pr is merged to fix it.

thisiswhereitype · 2017-07-17T16:37:54Z

Please see the graph implementation ready for review, as the commit message says there were some issues with the VHDL section so I have commented them out until they are resolved.
The graph implementation is now publicly exported through Pangraph but implemented in Pangraph.Internal.Graph I needed some helper functions for working with the other modules and have placed them in the former module.

snowleopard · 2017-07-17T17:16:08Z

@thisiswhereitype Wait, but VHDL code and tests are important, we can't simply comment them out.

If you can't make the new design work for them, it means the PR is not yet ready to be merged.

Also, the fact that you need access to some of the non-exported internals hints that the current API may not be powerful/flexible enough.

Let's solve both of the above problems. First: we need to make the VHDL code/tests work.

…ed in conversion to and from maps. Moved helper functions into the internal module. Added functions like and

…librarys have lots of fucntions for manipulating their respective instances

thisiswhereitype · 2017-07-19T13:05:10Z

Please could you provide feedback on the changes in Pangraph.Internal.Graph. I have removed the add and remove functions for the Pangraph type, instead the makePangraph verifies the graph is not malformed and has an Either return type accordingly. So users can extract from pangraph but not insert into it without constructing it again (at which point it is verified).

…her then returning either

snowleopard · 2017-07-19T14:48:45Z

@thisiswhereitype Thank you, I hope I'll find time to do the review tonight.

thisiswhereitype · 2017-07-19T16:27:03Z

You can leave it until tomorrow, I have other branches.

…m fucntion

snowleopard

There are quite a few but mostly minor changes.

snowleopard · 2017-07-23T18:56:23Z

fantasi/Tuura/Fantasi/Main.hs

-            let graphVHDL   = W2.writeGraphVhdl graphParsed
-            let simEnvVHDL  = W1.writeEnvironmentVhdl graphParsed
+      Right graphParsed -> case graphParsed of
+        Left malformedEdges -> error $ show malformedEdges


Why is the case with malformed edges not considered an error at the top level? Is this because you'd like to give the user opportunity to recover from this error without failing? I can see how this can be useful, but I don't like that the user now have to do two steps to finally get to the graph.

I suggest you do the following instead. You currently have:

graphmlToPangraph :: BS.ByteString -> Either BS.ByteString (Either [MalformedEdge] Pangraph)

What about this instead?

-- Let's add a data type to capture errors data PangraphError = ParseError BS.ByteString | MalformedEdges [MalformedEdge] | ... -- perhaps there are other types of errors -- Pangraph errors can be shown, so the user can just print them if need be instance Show PangraphError where ... -- Now the user can get the graph in one step: graphmlToPangraph :: BS.ByteString -> Either PangraphError Pangraph

snowleopard · 2017-07-23T18:57:58Z

src/Pangraph.hs

-    makeNodeWithID :: BS.ByteString -> Node
-    makeNodeWithID str = makeNode [makeAttribute ("id", str)]
+-- import Data.Map.Strict (Map)
+-- import qualified Data.Map.Strict                  as Map


No need to commit this to the repository.

snowleopard · 2017-07-23T18:58:33Z

src/Pangraph.hs

 ) where

+import Pangraph.Internal.Graph


Why do you need the internal module? Does this module do anything useful?

I remember you mentioned you had some issues with exporting the right functions. Have you added the internal module simply to solve the export problem? If yes, don't do that. Move everything to the top-level module.

snowleopard · 2017-07-23T19:01:59Z

src/Pangraph/GraphML/Parser.hs


-template:: [PT.Template]
-template = PT.graphMLTemplate
+graphmlToPangraph' :: BS.ByteString -> P.Pangraph


This is an unsafe function, and it's useful to mark such functions with the unsafe prefix.

Let's rename it to unsafeGraphmlToPangraph. Or, even better, to unsafeParse, because the return type (Pangraph) is clear from the type signature, and the GraphML should be the name of the module instead.

snowleopard · 2017-07-23T19:03:20Z

src/Pangraph/Internal/Graph.hs

+vertexByID,
+edgeByID,
+
+-- -- Operations on Pangraph


Don't commit commented out code to the repository. You can commit a comment instead, explaining that you plan to add more functions.

snowleopard · 2017-07-23T19:16:17Z

src/Pangraph/Internal/Graph.hs

+type Attribute = (Key, Value)
+type Key = BS.ByteString
+type Value = BS.ByteString
+type MalformedEdge = (Edge, (Maybe Vertex, Maybe Vertex))


Do you really need a separate type synonym for malformed edges? I think you are giving too much attention to malformed edges. It's fine to simply return the EdgeID of the first malformed edge, so I suggest you drop this functionality, because it makes the API more complex.

snowleopard · 2017-07-23T19:18:31Z

src/Pangraph/Internal/Graph.hs

+
+-- List based constructors
+
+makePangraph :: [Vertex] -> [Edge] -> Either [MalformedEdge] Pangraph


I would even go for [Vertex] -> [Edge] -> Maybe Pangraph in the first version of the API, explaining in the documentation that Nothing corresponds to the case where there is a malformed edge. No need to actually return it -- the user will be able to find it if need be.

snowleopard · 2017-07-23T19:20:36Z

src/Pangraph/VHDL/GraphWriter.hs

 import qualified Pangraph         as P
 import qualified Data.ByteString as BS
-import Data.ByteString.Char8 (unpack)
+import Data.ByteString.Char8 (unpack, pack)


 type NodeName  = String


I suggest to rename these type synonyms too and use "vertex" for consistency.

snowleopard · 2017-07-23T19:21:30Z

src/Pangraph/XMLTemplate.hs

@@ -3,71 +3,90 @@
 module Pangraph.XMLTemplate
 ( Template,
  graphMLTemplate,
-  parseTemplateToAlga,
-  parseTemplateToPangraph
+  -- parseTemplateToAlga,


Don't commit dead code, just delete this.

snowleopard · 2017-07-23T19:22:07Z

src/Pangraph/XMLTemplate.hs

-- A list of places to find nodes and edges.
-data Template = XML [Node] [Edge]
+-- A list of places to find nodes and extractEdges.
+data Template = XML [NodeRule] [EdgeRule]


Again, please use "vertex" throughout the repository to avoid confusion.

snowleopard · 2017-07-23T19:24:42Z

@thisiswhereitype Thanks for the PR! I've left a bunch of comments, please let me know if you have any questions. Ready to do the next review round.

…s, concrete implementations their instances are now in an internal file to avoid cyclic dependencies

thisiswhereitype · 2017-07-24T13:55:12Z

I have removed the dead code and added the errors as suggested.

In Pangraph.Internal.GraphType the core record types are exported into Pangraph.Internal.Graph and Pangraph.Internal.Error which then export the appropriate abstract types.
Pangraph then imports both and exports the external api. Meanwhile the other modules import Pangraph.Internal.Error in order to access error constructors which are not exported via Pangraph (the abstract types are however). So the only thing the user can do presently is show them.

I have not changed the makePangraph function (to Maybe Pangraph) yet as I think the issue is arising because the vertex and edge type are too tightly coupled between their data and implementation in data structures and this is making it hard to have separation of concerns between modules. Please could you review and I will elaborate if you are satisfied with these changes?

snowleopard · 2017-07-24T16:28:55Z

Meanwhile the other modules import Pangraph.Internal.Error in order to access error constructors which are not exported via Pangraph (the abstract types are however). So the only thing the user can do presently is show them.

I think this is wrong: the user should be able to pattern match on errors in order to appropriately handle them, so I suggest you move their definition to Pangraph and export them.

snowleopard · 2017-07-24T16:33:47Z

the vertex and edge type are too tightly coupled between their data and implementation in data structures and this is making it hard to have separation of concerns between modules.

I think your separation of Pangraph data structure into multiple modules is premature. I don't see much benefit yet, but it does introduce complexity, which seems unnecessary. Can you refactor into a single module? We can decompose later if need be.

Just to reiterate: we are going for the simplest possible solution that works. Once we have it, we'll proceed by refactoring it further (if need be).

snowleopard · 2017-07-27T20:40:11Z

Test.hs

-    showInstanceTestCase = ("Pangraph [Node [Attribute (\"id\",\"0\")],Node [Attribute (\"id\",\"1\")]] [Edge [Attribute (\"source\",\"0\"),Attribute (\"target\",\"1\")]]",
-     P.makePangraph [P.makeNode [P.makeAttribute ("id","0")],P.makeNode [P.makeAttribute ("id","1")]] [P.makeEdge [P.makeAttribute ("source","0"),P.makeAttribute ("target","1")]])
+testShowInstance = do
+  literal <-fmap (head . lines) $ readFile "test/show-string.txt"


I think we should avoid reading files during tests. There is no point to test readFile, we should focus on testing our own code instead.

This is a test that the show instance has not been changed between commits unexpectedly, I just moved string literal into the text file to remove clutter from the where clause.

Is the avoidance of IO to make it clear exactly what is being tested? I think maybe the test directory should mirror the src directory in terms of testing this will allow the files to be clearer and do less IO.

test/Pangraph.hs test/Pangraph/GraphML.hs /VHDL.hs

snowleopard · 2017-07-27T20:41:20Z

@thisiswhereitype Thanks! I have a few more comments. But I think we are getting close :-)

thisiswhereitype · 2017-08-04T11:40:27Z

As the commit messages say I have updated the testing using HUnit and created a CLT to convert files into Haskell String Literals.

snowleopard · 2017-08-04T13:32:04Z

stack.yaml

@@ -1,6 +1,6 @@
 extra-deps:
 resolver: lts-8.22
 flags: {}
-extra-package-dbs: []
+extra-package-dbs: [  ]


Leftover spaces?

thisiswhereitype · 2017-08-04T13:58:05Z

I have corrected the changes and added haddock markup to the API.

thisiswhereitype · 2017-08-07T14:16:59Z

Please review when you have chance, thank you.

snowleopard · 2017-08-09T14:09:11Z

src/Pangraph/Examples/Reading.hs

+
+import Data.ByteString (readFile)
+
+import qualified Pangraph.GraphML.Parser as GraphML_P


Use GraphML as the qualified name

snowleopard · 2017-08-09T14:18:46Z

src/Pangraph/VHDL/GraphWriter.hs


-type NodeName  = String
-type NodeIndex = Int
+-- | Writes a Pangraph to VHDL

 writeGraphVhdl :: P.Pangraph -> String


Name this simply write

I have opted to name these writeGraph and writeEnviroment and re-export these in Pangraph.VHDL which saves the user from using two imports. I also pack these into ByteStrings to save breaking changes later.

snowleopard · 2017-08-09T14:19:41Z

src/Pangraph/VHDL/GraphWriter.hs

+import Data.ByteString.Char8      (unpack)
+
+
+type VertexName  = String


Add a TODO here to switch to ByteString.

snowleopard · 2017-08-09T14:21:16Z

src/Pangraph/VHDL/GraphWriter.hs

-                         ++ "\tEND COMPONENT;\n\n"
-
-createSignals :: [P.Node] -> String
+-- createRegisterGeneric :: Int -> String


Let's add a TODO to convert these large literals into the following form:

s x = unlines [ "qwe" , "xru" ++ show x , "iop" ]

snowleopard · 2017-08-09T14:27:59Z

test/VHDLLiterals.hs

@@ -0,0 +1,3492 @@
+{-# LANGUAGE OverloadedStrings #-}


Let's not go beyond 10-20 vertices in hard-coded tests.

…moved large VHDL test case, corrected import syntax in some modules

thisiswhereitype · 2017-08-10T10:09:47Z

I have addressed all the concerns raised. Is the approach taken with the VHDL module acceptable?

snowleopard · 2017-08-10T12:04:19Z

fantasi/Tuura/Fantasi/Main.hs

-import qualified Pangraph.VHDL.EnvironmentWriter as W1
-import qualified Pangraph.VHDL.GraphWriter as W2
-import Data.ByteString.Char8 (pack)
+import qualified Pangraph.VHDL as V


Let's import as VHDL. If you see a V.foo in the code you'll need to guess what it means and/or look at the list of imports, whereas VHDL.foo is pretty self-explanatory.

snowleopard · 2017-08-10T12:07:05Z

@thisiswhereitype Thanks! Just one more tweak and I'm ready to merge.

…he project, made import names clearer

thisiswhereitype · 2017-08-10T14:52:49Z

I have made the change requested and I also moved the VHDL module into Pangraph.VHDL.Writer for consistent module naming. Please review @snowleopard.

snowleopard · 2017-08-10T15:21:57Z

@thisiswhereitype Looks good, thank you. I've merged the PR.

snowleopard · 2017-08-10T15:22:24Z

I haven't reviewed documentation -- please make sure it's up to date.

Joe added 3 commits June 28, 2017 17:00

updated readme

e0ada79

Spacing changes

aa2a7c2

Updated to simple interface, list based

fff5d44

thisiswhereitype requested a review from snowleopard July 14, 2017 12:49

thisiswhereitype mentioned this pull request Jul 14, 2017

Using algebraic-graphs #11

Closed

Removed old implementation and altered the current parser to use the …

ede942c

…new graph. VHDL section has been disabled but I commit again before the pr is merged to fix it.

Joe added 4 commits July 18, 2017 15:12

VHDL working again, most likely the list of nodes it used was reorder…

2520073

…ed in conversion to and from maps. Moved helper functions into the internal module. Added functions like and

split insertEdges into addEdge and updateEdge

4a2f7bc

Fixed git issues, last few commits contain details of this commit

aad0b01

Another iteration on the api, taking the idea that the maps and list …

645eb02

…librarys have lots of fucntions for manipulating their respective instances

Joe added 4 commits July 19, 2017 14:36

Applied hlint

4f24710

bump stack to lts-8.22

c004654

Update to readme and example

8518bd0

Added a second function which returns a pangraph or throws errors rat…

e00d071

…her then returning either

thisiswhereitype self-assigned this Jul 19, 2017

Changed vertices and edges function in Graph.hs to use Map.Strict ele…

73ed92f

…m fucntion

snowleopard requested changes Jul 23, 2017

View reviewed changes

Removed dead code, improved error handling with a sum type in Error.h…

83ee458

…s, concrete implementations their instances are now in an internal file to avoid cyclic dependencies

Changed Edge show implementation to make it compilable

288e7a1

snowleopard reviewed Jul 27, 2017

View reviewed changes

Joe added 2 commits July 28, 2017 12:40

Formatting changes in monadic code, bug fix for VHDL testing

58ad815

Used HUnit in testing, no file IO is required for testing the run now

b6011fb

snowleopard reviewed Aug 4, 2017

View reviewed changes

Joe added 2 commits August 4, 2017 14:52

Added Haddock, updates to readme

30143ab

Spacing Change

20ead9b

thisiswhereitype added 6 commits August 4, 2017 15:36

Fixed mistake in example

ab76a56

Bump to lts-9.00

ae4b278

Merge branch 'master' of github.com:thisiswhereitype/pangraph

2f0be80

Haddock typo

9afc3d2

Fixed all --wall warnings

d55bb02

Fixed --wall messages in test/

dd42b08

snowleopard reviewed Aug 9, 2017

View reviewed changes

test/VHDLLiterals.hs

@@ -0,0 +1,3492 @@

{-# LANGUAGE OverloadedStrings #-}

Copy link

Member

snowleopard Aug 9, 2017

Choose a reason for hiding this comment

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

Let's not go beyond 10-20 vertices in hard-coded tests.

VHDL now exported through Pangraph.VHDL with ByteString Interface, re…

46333ec

…moved large VHDL test case, corrected import syntax in some modules

snowleopard reviewed Aug 10, 2017

View reviewed changes

Moved the VHDL writer to have the same nomenclature as the reset of t…

dab67f7

…he project, made import names clearer

snowleopard merged commit 56da512 into tuura:master Aug 10, 2017


		-- List based constructors

		makePangraph :: [Vertex] -> [Edge] -> Either [MalformedEdge] Pangraph


		import Data.ByteString (readFile)

		import qualified Pangraph.GraphML.Parser as GraphML_P

		import Data.ByteString.Char8 (unpack)


		type VertexName = String

Implementing a simple graph api #14

Implementing a simple graph api #14

Conversation

thisiswhereitype commented Jul 14, 2017

thisiswhereitype commented Jul 14, 2017

snowleopard commented Jul 15, 2017

thisiswhereitype commented Jul 17, 2017

snowleopard commented Jul 17, 2017

thisiswhereitype commented Jul 19, 2017 • edited Loading

snowleopard commented Jul 19, 2017

thisiswhereitype commented Jul 19, 2017

snowleopard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowleopard commented Jul 23, 2017

thisiswhereitype commented Jul 24, 2017 • edited Loading

snowleopard commented Jul 24, 2017

snowleopard commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisiswhereitype Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

snowleopard commented Jul 27, 2017

thisiswhereitype commented Aug 4, 2017

Choose a reason for hiding this comment

thisiswhereitype commented Aug 4, 2017 • edited Loading

thisiswhereitype commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisiswhereitype commented Aug 10, 2017

Choose a reason for hiding this comment

snowleopard commented Aug 10, 2017

thisiswhereitype commented Aug 10, 2017 • edited Loading

snowleopard commented Aug 10, 2017

snowleopard commented Aug 10, 2017

thisiswhereitype commented Jul 19, 2017 •

edited

Loading

thisiswhereitype commented Jul 24, 2017 •

edited

Loading

thisiswhereitype Jul 28, 2017 •

edited

Loading

thisiswhereitype commented Aug 4, 2017 •

edited

Loading

thisiswhereitype commented Aug 10, 2017 •

edited

Loading