added back end / front end checks and tests for token field length (closes #168) #208
Conversation
app/utils/__init__.py
Outdated
|
|
||
|
|
||
| class PyrrhaError(Exception): | ||
| """Raised when errors specific to this application occur.""" | ||
| pass | ||
|
|
||
|
|
||
| class ValidationError(Exception): |
There was a problem hiding this comment.
Would it make sense to derive it from PyrrhaError ?
app/models/corpus.py
Outdated
| @@ -870,6 +871,9 @@ def add_batch(corpus_id, word_tokens_dict, context_left=None, context_right=None | |||
| corpus=corpus_id, | |||
| order_id=i+1 # Asked by JB Camps... | |||
| ) | |||
| list( | |||
| validate_length(k, v, lengths={"form": 64}) for k, v in wt.items() | |||
There was a problem hiding this comment.
I like the current system you built. The only thing I'd like more is to show the line number potentially ? But that would mean getting the error and showing a modified message
|
Looks good to me :) As for other fields, I see you have set up what's needed. I think there is less chances of a size constraint on other fields, and I wonder how the iteration over every key is gonna hurt performances. I'd probably feed the dictionary instead of key, value and iterate over the lengths Dict to avoid iterating on every object properties. |
carinedengler
changed the title
carinedengler
changed the title
|
hi, I updated the pull request with the front end checks and integrated your suggestions into the back end checks |
|
LGTM |
|
Looks good to me though I didn't see the error message because the top of the page where it is inserted is off screen. |
|
I was wondering just that about the error message - I put it where all of the other error messages are to be consistent with the rest of the interface, but made the focus on the line causing the error, I was wondering if an pop-up window would be more appropriate? |
|
Maybe a scroll to the message error would be simpler / cleaner ? |
|
after some quick tests, it seems that setSelectionRange only works when the focus is on the textArea, which is lost when the window is scrolled up intuitively, I think it's more user-friendly when the problematic line is highlighted for the user, but I can also understand if you prefer a consistent user interface |
|
I think scrolling to the problematic line is way better. If you find a solution for this, I'd be okay with it. |
|
Scrolling to the problematic line is ok (how does it behave with multiple errors?) yet the user need to understand that what is shown is an issue. I would suggest to put the error message much closer to the input form section. |
it highlights the last line containing an error and shows error messages for each faulty line (order needs to be fixed), I'm not sure if that's the best behavior though, I can think of two possible solutions
|
|
I like the second option :) I actually wonder, if the error message for each faulty line could have a link to the faulty line ? |
|
I implemented the second solution, and also rebased onto the current dev branch |
|
Given the proposed implementation it would be very easy to add the faulty line number. Thank your for the improvements, I'll merge this PR as soon as we get this (extra useful) info in the error message. |
|
I added the line number |
|
I think there might be a bug in the tests: https://travis-ci.org/github/hipster-philology/pyrrha/builds/732968766#L2778 |
|
I updated the error message in the test |
Hi,
I'm opening the pull request as a WIP for the back end part first, I wanted to confirm that this is what you had in mind, and if so, for which fields the validation should be done. Also, I was wondering if the same validation shouldn't be done for any other field having a length restriction in the database schema which is set using user input, such as the corpus name.