Implement exercise dot-dsl #978

ackerleytng · 2017-10-24T13:07:28Z

Will fix #732

ackerleytng · 2017-10-24T14:30:41Z

Okay, I think I'm pretty much done with implementing this exercise to close #732, please review it, thanks!!

cmccandless

Maintainers note: As of this time, there is no canonical data for this exercise.

Looks good to me!

cmccandless · 2017-10-24T15:47:01Z

exercises/dot-dsl/README.md

+
+Observe the test cases in `dot_dsl_test.py` to understand the DSL's design.
+
+### Submitting Exercises


Per #950 , this should be at heading level 2 instead of 3.

Moved it to level 2!

(as per exercism#950)

ackerleytng

Moved it to level 2!

ackerleytng · 2017-10-27T14:09:45Z

exercises/dot-dsl/README.md

+
+Observe the test cases in `dot_dsl_test.py` to understand the DSL's design.
+
+### Submitting Exercises


Moved it to level 2!

N-Parsons

Your implementation looks good, but I've left a few questions as comments.

N-Parsons · 2017-10-29T19:23:57Z

config.json

+        "maps",
+        "object_oriented_programming",
+        "test_driven_development",
+        "transforming"


What's the reasoning for including maps, test_driven_development and transforming?

When I left maps there, I was thinking of dictionaries. Now that I think of it again, I think maps probably means something else, in this case.

For object_oriented_programming, I thought that since we were using classes, it was fair to leave that as a topic.

For test_driven_development, I think there isn't any other way to complete this other than by reading through the test cases and building up the DSL, so I thought TDD is accurate here.

As for transforming, I thought it was fair to say that DSLs are a transformation of one representation of data to another.

Thanks for your explanation. I don't think that maps is a suitable topic here, but domain_specific_languages and graphs would probably be a sensible additions.

N-Parsons · 2017-10-29T19:32:39Z

exercises/dot-dsl/dot_dsl.py

+        self.attrs = attrs
+
+    def __eq__(self, other):
+        return self.name == other.name and self.attrs == other.attrs


I'm wondering whether it might be better not to implement this but to instead hint that it should be implemented? I feel like rather a lot of code is being supplied pre-written, without the user having to think about how to check for equality. I think that we should either set a lower difficulty for this problem, or we should add a .meta/hints.md that lets the user know that they need to implement the __eq__ method themself (with each method being stripped down to just pass).

I was concerned that without defining __eq__, the tests wouldn't even pass. A lot of the test cases, such as

self.assertEqual(g.nodes, [Node("a", {"color": "green"})])

requires __eq__ to be defined properly for the check to work.

Perhaps we could say that as part of TDD, the person working on this topic should seek to first complete __eq__ so that test cases work as the human would expect?

Looking at it again, I think it should be ok to leave it as it is - I'm not a great fan of supplying too much code, but the definition is also fairly easy and could border on being tedious.

N-Parsons · 2017-10-29T19:39:34Z

exercises/dot-dsl/example.py

+
+        for item in data:
+            if len(item) != 3 and len(item) != 4:
+                raise TypeError("Graph item malformed")


Is this check really necessary? Surely the conditionals below will achieve the same but with a more informative output?

hmm but without these checks,

type_ = item[0]

might raise an IndexError if len(item) < 1.

Or should I just say

if len(item) < 3: raise TypeError("Graph item incomplete")

?

Well spotted, I think that would be a sensible change.

N-Parsons · 2017-10-29T19:41:04Z

exercises/dot-dsl/example.py

+                    raise ValueError("EDGE malformed")
+                self.edges.append(Edge(item[1], item[2], item[3]))
+            else:
+                raise TypeError("Unknown item {}".format(item[0]))


Can you explain the reasoning for using a TypeError here instead of a ValueError here?

I changed it to a ValueError, thanks for pointing this out!

I thought about it and decided to follow the guidelines here

Fixed this in commit 812140a on the dot-dsl branch

Changed associated test cases

N-Parsons · 2017-11-11T23:38:53Z

exercises/dot-dsl/README.md

+
+Create a DSL similar to the dot language.
+
+## Description of DSL


This section should also go into a .meta/hints.md file so that it doesn't get lost if we regenerate the READMEs in the future.

N-Parsons · 2017-11-11T23:48:38Z

Sorry for taking a while to get back to this - I've had a rather busy couple of weeks!

There are a couple of minor issues that need to be resolved, and then this will be ready to merge:

Creation of .meta/hints.md (comment)
Change the input validation to avoid an IndexError (comment)
Update topics in config.json (comment)

N-Parsons

@ackerleytng Just a couple of minor changes that need to be made, and then this will be ready to merge.

N-Parsons · 2017-11-19T17:31:56Z

exercises/dot-dsl/README.md

-The implementations of `Node` and `Edge` provided in `dot_dsl.py`.
-
-Observe the test cases in `dot_dsl_test.py` to understand the DSL's design.
-


The this description should still be here! The contents of the .meta/hints.md file are put here when someone runs configlet generate to regenerate the READMEs.

Ok made the change!

N-Parsons · 2017-11-19T17:43:31Z

config.json

+        "test_driven_development",
+        "transforming"
+      ]
+    },


The current version of exercism goes through the exercises in the order specified in the config. I don't think that this should be a particularly early exercise, so can you move this section down to around line 750 or so (the exact location isn't too important).

Moved it lower!

N-Parsons · 2017-11-21T11:49:03Z

Looks good, thanks @ackerleytng!

ackerleytng added 5 commits October 24, 2017 21:06

Implement exercise dot-dsl

76044f9

Add working DSL implementation

894953e

Add more tests and checks for malformed inputs

1baf9c9

Move solution into example.py

6726f1f

Add explanation in README

c2986aa

ackerleytng added 2 commits October 24, 2017 22:34

Change code to avoid E129 from flake8

27f7c8b

Add entry for dot-dsl in config.json

dd62afe

cmccandless approved these changes Oct 24, 2017

View reviewed changes

cmccandless requested a review from ilya-khadykin October 24, 2017 15:51

ilya-khadykin changed the title ~~[WIP] Implement exercise dot-dsl~~ Implement exercise dot-dsl Oct 25, 2017

ilya-khadykin requested a review from N-Parsons October 25, 2017 06:52

ilya-khadykin added maintainer action required labels Oct 25, 2017

Move Submitting Exercises to level 2

095b711

(as per exercism#950)

ackerleytng commented Oct 27, 2017

View reviewed changes

N-Parsons reviewed Oct 29, 2017

View reviewed changes

Update TypeError to ValueError

812140a

Changed associated test cases

N-Parsons reviewed Nov 11, 2017

View reviewed changes

ackerleytng added 3 commits November 19, 2017 21:46

Move description of DSL to .meta/hints.md

e6fec30

Change input validation to avoid IndexError

45da0f9

Update topics in config.json

3ee16a1

N-Parsons suggested changes Nov 19, 2017

View reviewed changes

ackerleytng added 2 commits November 20, 2017 20:16

Reorder exercise in config.json

3a98b60

Add description of DSL into README.md

e778b6d

N-Parsons approved these changes Nov 21, 2017

View reviewed changes

Merge branch 'master' into dot-dsl

c45c50d

N-Parsons merged commit 5a81d6a into exercism:master Nov 21, 2017

N-Parsons pushed a commit to N-Parsons/exercism-python that referenced this pull request Nov 21, 2017

Implement exercise dot-dsl (exercism#978)

31f9494

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement exercise dot-dsl #978

Implement exercise dot-dsl #978

ackerleytng commented Oct 24, 2017 •

edited

Loading

ackerleytng commented Oct 24, 2017 •

edited

Loading

cmccandless left a comment

cmccandless Oct 24, 2017

ackerleytng Oct 27, 2017

ackerleytng left a comment

ackerleytng Oct 27, 2017

N-Parsons left a comment

N-Parsons Oct 29, 2017

ackerleytng Nov 1, 2017

N-Parsons Nov 11, 2017

ackerleytng Nov 19, 2017

N-Parsons Oct 29, 2017

ackerleytng Nov 1, 2017

N-Parsons Nov 11, 2017

N-Parsons Oct 29, 2017

ackerleytng Nov 1, 2017

N-Parsons Nov 11, 2017

ackerleytng Nov 19, 2017

N-Parsons Oct 29, 2017

ackerleytng Nov 1, 2017

N-Parsons Nov 11, 2017

ackerleytng Nov 19, 2017

N-Parsons commented Nov 11, 2017

N-Parsons left a comment

N-Parsons Nov 19, 2017

ackerleytng Nov 20, 2017

N-Parsons Nov 19, 2017

ackerleytng Nov 20, 2017

N-Parsons commented Nov 21, 2017


		Observe the test cases in `dot_dsl_test.py` to understand the DSL's design.

		### Submitting Exercises


		Create a DSL similar to the dot language.

		## Description of DSL

		The implementations of `Node` and `Edge` provided in `dot_dsl.py`.

		Observe the test cases in `dot_dsl_test.py` to understand the DSL's design.

Implement exercise dot-dsl #978

Implement exercise dot-dsl #978

Conversation

ackerleytng commented Oct 24, 2017 • edited Loading

ackerleytng commented Oct 24, 2017 • edited Loading

cmccandless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ackerleytng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-Parsons 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

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

N-Parsons commented Nov 11, 2017

N-Parsons 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

N-Parsons commented Nov 21, 2017

ackerleytng commented Oct 24, 2017 •

edited

Loading

ackerleytng commented Oct 24, 2017 •

edited

Loading