Skip to content
This repository has been archived by the owner on Nov 28, 2024. It is now read-only.

Report an error if schema isn't loaded #186

Open
ldodds opened this issue Jun 2, 2015 · 10 comments
Open

Report an error if schema isn't loaded #186

ldodds opened this issue Jun 2, 2015 · 10 comments

Comments

@ldodds
Copy link
Contributor

ldodds commented Jun 2, 2015

If a user supplies a schema and we fail to load it, then the application should report an error so its clear that the schema hasn't been applied.

Currently it falls back to not applying any schema validation which is misleading.

As a first step, reporting a basic error would be an immediate improvement.

Reporting why the schema is invalid would also be useful, although there are a range of possible errors:

  • invalid JSON
  • invalid regex
  • invalid values for a constraint specification, e.g. a number or string where true/false is expected
  • unknown constraint

The first two are definitely fatal errors. The last two are potentially just warnings. Depends if we want to work on a "best effort" basis.

bcouston pushed a commit that referenced this issue Jul 8, 2015
Fix of error where uploading an invalid .json schema  (empty or
corrupted) from local filestore did not trigger an invalid JSON warning.
@bcouston
Copy link
Contributor

bcouston commented Jul 9, 2015

I'll be taking invalid regex

@quadrophobiac
Copy link
Contributor

change error message for schema error, currently prints "Sorry, your CSV did not pass validation. Please review the errors and warnings below:"

@quadrophobiac
Copy link
Contributor

lines 26 - 34 of Validation currently always eval false, would be worthwhile working out what condition they were intended to catch

    if io.respond_to?(:tempfile)
      filename = io.original_filename
      csv = File.new(io.tempfile)
      io = File.new(io.tempfile)
    elsif io.class == Hash && !io[:body].nil?
      filename = io[:filename]
      csv_id = io[:csv_id]
      io = StringIO.new(io[:body])
    end

@Floppy
Copy link
Member

Floppy commented Jul 10, 2015

@quadrophobiac you mean both of these conditions always fail? The first looks like it's intended to handle uploaded files, and the second ... not sure. Will look at the context....

@Floppy
Copy link
Member

Floppy commented Jul 10, 2015

@quadrophobiac do you mean this section? https://github.com/theodi/csvlint/blob/master/app/models/validation.rb#L19

The line numbers seems different...

@quadrophobiac
Copy link
Contributor

Yes apologies - I was referring to the line numbers on the error-reporting-schema_expanded-test-suite branch

@quadrophobiac
Copy link
Contributor

I've confirmed non triggering by running the tests suite with a byebug in each conditional

@Floppy
Copy link
Member

Floppy commented Jul 10, 2015

Ah, I looked on the fix-schema branch but not the other - my bad. Looking closer now.

@bcouston
Copy link
Contributor

Started a pull request on invalid regex functionality: Data-Liberation-Front/csvlint.rb#132

@pezholio
Copy link
Contributor

Data-Liberation-Front/csvlint.rb#132 is good to go - so we need to make sure the newly created errors are shown this end

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants