search

lutaml / expressir

Ruby parser for the ISO EXPRESS language
3 stars 3 forks source link

Should we wrap `NameError` to `Expressir::Error`? #118

Closed CAMOBAP closed 2 years ago

CAMOBAP commented 2 years ago

Related to https://github.com/lutaml/lutaml/pull/35#issuecomment-1124929861

Intro

expressir@1.1.0 contains breaking changes (Schema class moved under Declaration submodule)

As result during parsing, it may throws NameError: uninitialized constant Expressir::Model::Schema for old files

Proposal

I think more elegant way is to throw Expressir::Error instead NameError: uninitialized constant Expressir::Model::Schema

maxirmx commented 2 years ago

@CAMOBAP @ronaldtse I agree that throwing Expressir::Error would be reasonable though it is not directly or indirectly related to the changes I made in https://github.com/lutaml/expressir/pull/116.

I think expressir@1.1.0 introduced semantic changes while I was trying to avoid such changes by all possible means

CAMOBAP commented 2 years ago

@maxirmx yep absolutely (there is no relation to #116)

ronaldtse commented 2 years ago

@maxirmx could you help handle when you have time? Thanks!

maxirmx commented 2 years ago

@CAMOBAP @ronaldtse I have double checked it. To my best understanding Expressir throws Expressir::Error in the case under discussion and there is a test for it https://github.com/lutaml/expressir/blob/ce2e4568607c15c391df6e15dca67228ccd68041/spec/expressir/express/cache_spec.rb#L59

lutaml also has such test https://github.com/lutaml/lutaml/blob/e4ee4b43f916fa6423ce63735211672af7b3ea92/spec/lutaml/parser_spec.rb#L42 but in lutaml spec file there are errors in calls to Expressir::Express::Cache.to_file Expressir::Express::Cache.from_file

So I have fixed lutaml tests in this PR: https://github.com/lutaml/lutaml/pull/36

CAMOBAP commented 2 years ago

To my best understanding Expressir throws Expressir::Error in the case under discussion and there is a test for it

@maxirmx Not always. In this particular case breaking changes (Schema class moved under Declaration submodule) NameError was thrown during obvious reason (the class was renamed)

In my understanding, we should handle such cases and 'wrap' NameError with Expressir::Error. probably make sens to wrap all NameError for from_file call

maxirmx commented 2 years ago

@CAMOBAP

What I found:

  1. There is a couple of issues in lutaml specs when they pass an array of Ruby File objects to Expressir...from_files. from_files expects an array of strings(paths) as an argument. It caused some strange exceptions in rake spec but after I fixed the issues spect has been working like charm.
  2. Correct behavior of Expressir::Express::Cache.from_file that throws Expressir::Error in case of version mismatch

I feel that you are talking about some other scenario but I just do not understand it. Sorry for it. Do you mean that the repo was saved to yaml directly (i.e.: not using Expressir::Express::Cache) by the older version and then failed to be loaded the newer version ?

CAMOBAP commented 2 years ago

Do you mean that the repo was saved to YAML directly (i.e.: not using Expressir::Express::Cache) by the older version and then failed to be loaded in the newer version?

Yep, but as far as I understood YAML was obtained by saving through Expressir::Express::Cache. But this YAML was saved before Expressir::Model::Schema class was renamed to Expressir::Model::Declarations::Schema. This is why for old version it works and for new one expressir throws NameError: uninitialized constant Expressir::Model::Schema

maxirmx commented 2 years ago

In the current version Expressir::Express::Cache stores schema version as the first field. When cache is loaded Expressir::Express::Cache first checks this version number and throws Expressir::Error in case of mismatch. So I believe the issue you describe cannot happen with the current version though it has been fixed in a different way.

CAMOBAP commented 2 years ago

@maxirmx thanks for explanation it's totally make sens, and in this case there is nothing to fix, closing this one