Add support for certain unicode characters in ot names#234
Conversation
| response = self.client.post(f'{url}?name={name}') | ||
| self.assertEqual(response.status_code, 201) | ||
|
|
||
| def test_name_in_full_list(self): |
There was a problem hiding this comment.
this test name is weird. I don't fully understand what it does without the docstring.
There was a problem hiding this comment.
I agree and it doesn't look like it belong under creation test anyway. In fact, it looks quite similar to test_returns_name_in_list
There was a problem hiding this comment.
Let's just rename it to something that makes sense, then.
There was a problem hiding this comment.
I was trying to say that this test looks redundant.
There was a problem hiding this comment.
Aha. Okay well I'd be open to you just scrapping it, too, then.
|
this looks excellent. |
|
03a08db was pushed by @jchristgit despite this PR being open (and made me realise this PR lacks a migration). What should I do with this PR? Make it a test-only PR? In that case I should probably clean up the history with a force push 👿. |
Let's revitalize this PR. I think it's a good idea to have a couple of test cases in place for that regex validation. I know it's not a strict requirement for the test coverage in terms of lines touched during testing, but it would be beneficial in terms of feature coverage. |
|
@SebastiaanZ Not sure what you mean. I already improved on the tests that were there. Did you have something more in mind? |
|
@MarkKoz No, but you asked what you had to do with this PR and it was getting stale. I tried answering that question by saying that I think that your tests are still a useful addition to the site, but I guess my wording threw you off. It's currently conflicting with master, though, because of the now redundant changes in the model file. I wouldn't be opposed to you cleaning up the history in this case, since I don't think it's going to complicate reviews. |
MarkKoz
force-pushed
the
django-ot-names
branch
from
135ee87 to
7b3af30
Compare
|
This should be good to go now. |
SebastiaanZ
left a comment
SebastiaanZ
left a comment
There was a problem hiding this comment.
lgtm. Tested locally and everything works.
3 participants
No description provided.