Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Invalid schema #132

Merged
merged 7 commits into from
Jul 24, 2015
Merged

Invalid schema #132

merged 7 commits into from
Jul 24, 2015

Conversation

bcouston
Copy link

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 added 2 commits July 10, 2015 17:47
Test addition for when the expression in pattern in constraints for any
field is not in valid regular expression form.
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
Copy link
Author

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.

bcouston added 3 commits July 13, 2015 16:04
Some explaining of existing functionality as well as my previous
commits concerning regular expression structure validation.
@pezholio
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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

@bcouston
Copy link
Author

[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
Copy link
Contributor

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
Copy link
Author

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

@pezholio
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

Cool cool, is this good to go then?

@bcouston
Copy link
Author

@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
Copy link
Contributor

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

@bcouston
Copy link
Author

@pezholio on it

Invalid Regex error now shows only once per a field with a pattern
constraint that contains invalid regular expression form in the schema.
Also skips reporting unmatching RegEx form between CSV and schema in
that column when the error is found.
@bcouston
Copy link
Author

DO NOT MERGE until reviewed further.

Fixes previously failing tests.
@pezholio
Copy link
Contributor

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

pezholio added a commit that referenced this pull request Jul 24, 2015
@pezholio pezholio merged commit 9dc91dc into master Jul 24, 2015
@pezholio pezholio deleted the invalid-schema branch July 24, 2015 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants