- Notifications
You must be signed in to change notification settings - Fork51
Draft: (partial) fix for 'ce.declare' with SymbolDefinition using prop. 'type'#226
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| !( | ||
| (typeofdef.type==='string'&&isSubtype(def.type,'function'))|| | ||
| (def.type&&isValidType(def.type)) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Think this was a bug/typo? I.e. final cond. to be negated.
| thrownewError( | ||
| 'The `type` field of a symbol definition should be of type `string`' | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Think that's OK; but some introduced errs. may be surfacing on account oftype being given asundefined: so may require minor adjstment.
Uh oh!
There was an error while loading.Please reload this page.
Noticed that
ce.declareerrors when supplying aSymbolDefinition of form{type: TypeString}for second arg.Current changes appear to address the issue, but appears tosurface or reveal a few existing test errs. where throughout these, type values given through 'SymbolDefinition.type' do not conform to current type definitions: e.g.
{ type: 'set<integers>', ... }is given whereas in this context, according to definition of the type 'Type', a{kind: 'set', elements: 'integers' }is expected instead. Did try to investigate further but opened a bit of a can of worms.Believe that almost all errs. can be addressed fairly easily with present changes, but may require some alterations.
For reference, for the commit at which this was branched, 4 'snapshot' errors were already present split between tests 'compute-engine/compile.test.ts', and 'compute-engine/compile.test.ts'. This change surfaces ~18 newer. Feel free to edit as pleased or denounce changes