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

Allow for creating a LookupDef without manually initializing a ModelListing #501

Closed
chrispcampbell opened this issue Jun 11, 2024 · 0 comments · Fixed by #502 or #476
Closed

Allow for creating a LookupDef without manually initializing a ModelListing #501

chrispcampbell opened this issue Jun 11, 2024 · 0 comments · Fixed by #502 or #476

Comments

@chrispcampbell
Copy link
Contributor

chrispcampbell commented Jun 11, 2024

One thing I didn't like about the recent changes to allow overriding lookup data (issue #472, PR #490) is that it requires the developer to create a ModelListing instance at runtime and manually find a VarSpec for the purposes of identifying the variable to be overridden.

Both ModelListing and VarSpec were previously implementation (or hidden API) details that were meant for advanced use cases (like in testing tools for accessing implementation variables), and I think it was a mistake to make developers have to understand these details.

It would be more convenient for developers if they can simply provide a variable name or ID to identify the variable.

Current:

  // Read the JSON model listing
  const listingJson = await readFile(joinPath('sde-prep', 'build', 'processed.json'), 'utf8')
  const listing = new ModelListing(listingJson)

  // Find the `VarSpec` that corresponds to a certain variable
  const lookupVarSpec = listing.varSpecs.get('_a_data[_a1]')

  // Create a `LookupDef` to override data for a variable
  const lookupDef = createLookupDef(lookupVarSpec, [...])

Proposed:

  // Create a `LookupDef` to override data for a variable (can now simply provide the variable name)
  const lookupDef = createLookupDef({ varName: 'A data[A1]' }, [...])

  // Or you can provide the variable ID
  const lookupDef = createLookupDef({ varId: '_a_data[_a1]' }, [...])

For this to work, it will be necessary to bundle a minimal model listing with the generated model code. This feature (overriding lookup data) isn't likely to be needed for many models and bundling the listing would increase the size of the generated code, so I think I will make it an opt-in thing for now.

Once this change is implemented, ModelListing and VarSpec can go back to being implementation details and can be hidden from the public API for the time being.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment