Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I had to merge into master and then redo a new pull request, since without the gitignore being applied, there were 1444 files to traverse in the adapter_env folder (!!!!!!!) making it sort of impossible to find the adapter file and test files.... Hope its ok!
The to_json test does not pass, and it is probably due to the following concerns
Concerns and Changes
dictionaries CANNOT access their items through dot notation apparently. use the model["nodes"] syntax.
(I think it has to do with dot notation accessing attributes of the dictionary "class" making it not work on contained values)
By just changing the nodes to the new syntax, they are correct in the test (however, edges still fail)
fixtures in conftest has been updated to follow the new assumption (each road consists of two edges)
Remember that the adapter only gets table data as input. Weights, error_list, etc. should be set to a default value
(as they are not part of a database entry)
i made a "win_env" since apparently the mac installation ruined the windows one... There is a fix, but its not worth the effort to fix right now (we have more pressing concerns)
Suggestion
In general, i will suggest to run pytest as a command often.
And then i have a suggestion, that each edge should NOT have its own unique ID.
consider the following
model["nodes"][node_one]["edges"][node_two]
This gives the road, connecting from node one to node two, uniquely identifying it.
However, if the edge has an id of, for instance 5, this value cannot be queried by our model, other than by making two consecutive for loops (we have no idea where the "5." edge is in our list of nodes)
As such, i believe we have a fine primary key, consisting of two nodes giving a unique road, and that giving each road an id would only give an overhead, which would never be used in the best case, and would give rise to anti-patterns in the worst (querying two for loops for instance)
I'm open for counterarguments, but would like to be presented with a query using the edge_id, if there still is insistance for the id.