Custom Weather: Validation #3313
Conversation
Previously upload button's state was set in two different places: turned off during a fetch, and turned back on during error handling. This was because we assumed that in the success case the entire view would be replaced with Existing Weather Data View. However, in case of re-uploading after validation errors, the button needs to be turned back on. Furthermore, it is better to consolidate all the logic concerning it in one place makes it more composable.
|
Fixing the lint ... |
There was a problem hiding this comment.
Very good accounting of the edge cases here. I did find one aspect that will report false negatives if there are certain errors, but none that allow invalid data to be accepted. The false negatives would be resolved if the user fixed the valid errors, but my concern is that they might prove to be red herring that confuse the user. I'm fine with deferring a fix for that if it's not trivial, because the user should be able to understand the issue by looking at the source/error messages.
I also added a comment to #3312 about a workflow that, while not really a bug (the user doesn't hit save), may leave CWD files attached to a scenario that the user doesn't expect to be there.
Nice work.
| # line numbers start at 1, and we need to account for the header. | ||
| err(e.message, idx + 2) | ||
|
|
||
| previous_d = d |
There was a problem hiding this comment.
This may not be problematic enough to address, but you'll introduce an "incorrect date sequence" error for every other error that may be contained in the list. This may be confusing if there are a few "real" errors, but they start trying to fix their date sequences. The problem is that the previous_d is incremented even if the row fails, so the next "correct" row looks like it should be incremented an addition date - but really the previous row meant to capture that date. You can see the fact that the follow up line-error is consistent to the real errors:
There was a problem hiding this comment.
I think this is tolerable for now. I've recorded this in the enhancement card #3316.
There was a problem hiding this comment.
The reason I think this is tolerable is that I hope we are communicating well enough that we need contiguous dates of data, and when the user is fixing the file they may notice and ensure that the dates are contiguous, fixing all of this together.
Yet, attention narrows as matter of course, and this case may yet become annoying enough for us to solve. But given the budget I propose we wait to hear from users before fixing it.
|
Thanks for taking a look! Going to squash and merge now. |
The `err` helper method is used as shorthand for reporting errors. The first parameter is the message, the second is the line number. If specified, the line number will be prefixed to the error message. The validations check for the following: - Valid CSV header - Valid length of file - Valid year range between first and last date - All dates are properly parseable - There are no duplicate dates - There are no missing / out of order dates - Precipitation and temperature are valid floats - Precipitation is not negative If a file has too many errors, we only report that first 10. This is configurable in the MAX_ERRORS variable.
rajadain
force-pushed
the
tt/custom-weather-validation
branch
from
8af32b4
to
4e9e3ae
Compare
Overview
Adds, enhances validations for custom weather uploads.
The
errhelper method is used as shorthand for reporting errors. The first parameter is the message, the second is the line number. If specified, the line number will be prefixed to the error message.The validations check for the following:
If a file has too many errors, we only report that first 10. This is configurable in the MAX_ERRORS variable.
Connects #3284
Demo
Notes
If you find any unrelated bugs during the testing, please add them to #3312.
Testing Instructions
bundle --debugfull-3-yearsandfull-30-yearsfull-3-yearsandfull-30-years