Skip to content
This repository has been archived by the owner on Apr 21, 2021. It is now read-only.

Escape string quotes #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Escape string quotes #9

wants to merge 2 commits into from

Conversation

Lyla-Fischer
Copy link
Collaborator

@Lyla-Fischer Lyla-Fischer commented Jun 1, 2019

First draft of string escaping. I put the appropriate code in the three places where there seems to be repeated code, and I think that I should probably take the opportunity to refactor that code to be more DRY. Submitting this as the quick-and-dirty fix, though.

Addresses #7

@Lyla-Fischer
Copy link
Collaborator Author

lol - you used to be able to get away with force-pushing on pull requests without Github commenting on it.

After trying to enter the resulting mutation into dgraph, I found out that I was misformating it (you're not supposed to escape single-quotes), so I removed that part of the escaping.

@Lyla-Fischer
Copy link
Collaborator Author

I'm embarrassed at the force-pushing, but in other projects, that was preferred to having a commit history with a separate commit for every mistake made, at least when we were in the pull request review stage. Let me know if you prefer something different for your project.

@ahopkins
Copy link
Owner

ahopkins commented Jun 1, 2019

My preferred method would be a WIP: prefix to the PR name. And commit away. At the end when it is ready a rebase -i to cleanup anything unnecessary.

@Lyla-Fischer
Copy link
Collaborator Author

The tests pass now, and I tested the output in dgraph to make sure that it runs.

@ahopkins
Copy link
Owner

ahopkins commented Jun 2, 2019

nice. Any more work on this? Can I merge? If I have a chance to setup Travis CI this week I will. It might have to wait till next week. I will be at PyCon Israel this week.

@Lyla-Fischer
Copy link
Collaborator Author

Ya, I think it's ready to go.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants