Data-Liberation-Front / csvlint.rb

The gem behind http://csvlint.io
MIT License
283 stars 86 forks source link

Invalid schema #132

Closed bcouston closed 9 years ago

bcouston commented 9 years ago

Replacement of conditional statement to see if a regular expression was created with a RegexpError rescue to catch when creation failed and act accordingly.

bcouston commented 9 years ago

DO NOT MERGE. Tests pass, but app gets stuck in an endless loop if you try to validate a schema with an invalid regular expression pattern. Need some help from @Floppy or @pezholio to pin this down.

pezholio commented 9 years ago

Hey @bcouston - try running bundle exec rake jobs:work as well as the app server. Validations are carried out as background jobs to prevent requests timing out on the frontend, The app then polls an endpoint, which then redirects to the validation once it's been created.

bcouston commented 9 years ago

@pezholio I'm still getting a hang up, it keeps making the same request, any ideas? Validations seem to be empty. screen shot 2015-07-14 at 09 53 57

pezholio commented 9 years ago

Yeah, it should keep polling that request until the validation is created. What does the console output of rake jobs:work say?

bcouston commented 9 years ago

[Worker(host:Bens-MacBook-Air.local pid:45708)] Job Package#create_package (id=55a4cdb442656eb33c000003) FAILED (4 prior attempts) with RegexpError: premature end of char-class: /[/

So the RegexpError is causing it to hang, can I get the app to execute the code in the rescue block and carry on with normal operation? @pezholio

pezholio commented 9 years ago

Ah, looks like the Regexp error is happening in the app, rather than here. I think catching that with a begin ... rescue block should be fine

bcouston commented 9 years ago

link text @pezholio I've already implemented one, is there anywhere I've made a mistake?

pezholio commented 9 years ago

Ah, no, the code here is fine. It seems that there's an error happening in the CSVlint app. I'll have a dig.

bcouston commented 9 years ago

I just had change the link for the csvlint gem in the Gemfile in csvlint to refer the branch I was working on (invalid-schema)

pezholio commented 9 years ago

Cool cool, is this good to go then?

bcouston commented 9 years ago

@pezholio at the moment, it raises an invalid regular expression error for every record of the field in the csv that doesn't match the invalid regex (so every one). It would be a a lot cleaner if it was raised once and other errors/warnings concerning regex were not displayed, as an invalid regex form in the schema probably takes priority. Thoughts?

pezholio commented 9 years ago

Yeah, I think it's only sufficient to show the error once

bcouston commented 9 years ago

@pezholio on it

bcouston commented 9 years ago

DO NOT MERGE until reviewed further.

pezholio commented 9 years ago

Cool, this looks good now. We'll need to add support for this in the Csvlint app too, so I won't merge just yet, especially as we have a few more PRs open