Noah Marcus DataMade Code Challenge by noah-marcus · Pull Request #1 · noah-marcus/code-challenge

noah-marcus · 2020-09-22T21:35:42Z

DataMade Code Challenge Submission

This is my submission for the DataMade Code Challenge!

beamalsky

Hey @noah-marcus! I'm Bea, and I'll be reviewing your code challenge and co-interviewing you with Derek this week.

Lots of good stuff here. I found your code clear to read and appreciated the thorough commenting on your thought processes, though I'd like to see explicit testing instructions in your README. I left a few additional comments inline, and a couple ways you could improve your error handling.

We'd like you to respond to the review and make the requested changes before your interview, if time allows. For guidance, we like and try to stick to Thoughtbot's guide to code review when working together on pull requests.

Let me know in the relevant thread if you have any questions, etc., and I'll be sure to get back to you ASAP. Look forward to talking more tomorrow.

beamalsky · 2020-10-13T15:49:18Z

README.md

-
-**Note:** You can use the following address strings for testing during implementation:
-
- ✅ Valid: `123 main st chicago il`


In condensing this README, you lost the testing instructions! Could you add these test strings back, either in this README or in your PR description? For reference, here's DataMade's pull request template.

Whoops! Thanks for catching that. I added it back into the README. Change has been committed here.

beamalsky · 2020-10-13T15:49:58Z

README.md

-returns the expected output.
-
-You can run the tests using Docker:
+You can run some tests using Docker:


We'd appreciate a description of precisely what these tests are looking for. Could you expand this section?

Sure thing. I added some more details here. Please let me know if you think I should add more detail!

beamalsky · 2020-10-13T15:50:24Z

parserator_web/templates/parserator_web/index.html

        </table>
      </div>
+      <div id="parse-error" class="alert alert-danger mt-2 text-center" style="display:none" role="alert">
+        Uh-oh -- The inputted address could not be parsed!


Nice job anticipating the edge case of an empty string submission 👍

beamalsky · 2020-10-13T16:02:37Z

parserator_web/views.py

 class AddressParse(APIView):
    renderer_classes = [JSONRenderer]

+    # parse requested address string and return components to frontend 


Nice job documenting the types, but this could be a docstring! Could you create one for both this function and parse()?

Thank you for pointing that out, my knowledge of Python conventions is not my strongest asset 😳. Here is my attempt at converting my comments to docstrings.

beamalsky · 2020-10-13T16:03:44Z

tests/test_views.py

    address_string = '123 main st chicago il'
-    pytest.fail()
+
+    expected_return_input_string = '123 main st chicago il'


Why do you need this assigned separately from address_string above?

Yeah, I understand why that is confusing. I made that choice for my own visual preference. When writing my tests, I like to compartmentalize what I am giving to the test and what I expect in return.

Since there is an intended overlap (the expected return string should be the given address string), my expected_return_input_string is redundant and does not need to be assigned separately. I'll fix that up!

Edit: Change has been committed!

beamalsky · 2020-10-13T16:04:35Z

tests/test_views.py

+
+    expected_return_input_string = '123 main st chicago il'
+    expected_return_address_components = OrderedDict([('AddressNumber', '123'), ('StreetName', 'main'), ('StreetNamePostType', 'st'), ('PlaceName', 'chicago'), ('StateName', 'il')])
+    excpected_return_address_type = 'Street Address'


Looks like you've got a typo in the variable name here.

Good catch, I need to install a spell checker into my IDE! (Updated here)

beamalsky · 2020-10-13T16:05:20Z

tests/test_views.py

+    assert response.data['input_string'] == expected_return_input_string
+    assert response.data['address_components'] == expected_return_address_components
+    assert response.data['address_type'] == excpected_return_address_type
+    # pytest.fail()


You don't need this commented-out code.

👍 (Updated here)

beamalsky · 2020-10-13T16:28:47Z

parserator_web/templates/parserator_web/index.html

+      var input_string = $('#address').val();
+
+      // only parse input if user inputted something
+      if (input_string != ""){


If a user submits a string of spaces (eg, ' '), this is the result:

How can you handle that case so that this submit function treats it as an empty string?

I would have never thought of that edge case, thanks! I added .trim() to when I capture the input. Commit is here.

Looks good 👍

beamalsky · 2020-10-13T16:44:46Z

parserator_web/views.py

+        # pass request address to parse method
+        try:
+            address_components, address_type = self.parse(input_string)
+        except Exception as e:


If you look at e here there are more details on the error. For example, by inserting print(str(e)) in this except block and submitting 123 main st chicago il 123 main to the form I can see:

parserator | ERROR: Unable to tag this string because more than one area of the string has the same label parserator | parserator | ORIGINAL STRING: 123 main st chicago il 123 main parserator | PARSED TOKENS: [('123', 'AddressNumber'), ('main', 'StreetName'), ('st', 'StreetNamePostType'), ('chicago', 'PlaceName'), ('il', 'StateName'), ('123', 'AddressNumber'), ('main', 'StreetName')] parserator | UNCERTAIN LABEL: AddressNumber parserator | parserator | When this error is raised, it's likely that either (1) the string is not a valid person/corporation name or (2) some tokens were labeled incorrectly parserator | parserator | To report an error in labeling a valid name, open an issue at https://github.com/datamade/usaddress/issues/new - it'll help us continue to improve probablepeople! parserator | parserator | For more information, see the documentation at https://usaddress.readthedocs.io/

Could you capture this error message and display it on the frontend in a way that's useful to a user?

Yeah, I was struggling with figuring out what message to pass on to the user and was not sure how much information was too much. In this commit here, I am now capturing the entire message and displaying it on the frontend. I think the benefit to this approach is that the user will always receive a detailed and specific message from the backend.

Also, I'm not sure how to capture this new message for the unit test check? The commit above is a work in progress because the error handling is failing. @beamalsky, any advice?

There's some room for personal style here, and you don't know who your users are in this theoretical app so what you included as an error message is definitely reasonable.

One way I've handled returning the full error code without overwhelming users is to have a human-friendly error on top, and then the error stashed in under a "See more" collapsed section.

Instead of returning a ParseError for a bad request, you could return a Response with a 400 error code. Something like this:

return Response({ 'input_string': addr, 'address_components': '', 'address_type': '', 'error' : str(e)}, status=400 )

That way, you'll have access to the full error text from usaddress under error and a Response to test the attributes of in your test in a way that mirrors successful submission.

That makes a lot of sense, thanks for clearing that up! I updated my code in this commit and the test is working again!

Noah Marcus added 4 commits September 13, 2020 22:18

WIP: completed steps one - three of coding challenge

1be9db5

completed step 4, added error handling from step 3

59529e8

added unit tests (step 5)

9d3c7b5

clean up error handling and readme

9ed9618

beamalsky suggested changes Oct 13, 2020

View reviewed changes


		Note: You can use the following address strings for testing during implementation:

		- ✅ Valid: `123 main st chicago il`

Conversation

noah-marcus commented Sep 22, 2020

DataMade Code Challenge Submission

Uh oh!

beamalsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noah-marcus Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

noah-marcus Oct 13, 2020 •

edited

Loading