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

Remove duplicate implementations of canonical[Var]Name functions #578

Closed
chrispcampbell opened this issue Dec 7, 2024 · 1 comment · Fixed by #580 or #577
Closed

Remove duplicate implementations of canonical[Var]Name functions #578

chrispcampbell opened this issue Dec 7, 2024 · 1 comment · Fixed by #580 or #577

Comments

@chrispcampbell
Copy link
Contributor

Originally (like, a few years ago before I added the parse and various plugin-* packages) we had one implementation each of the canonicalName and canonicalVensimName functions in the compile package.

Over time I copied these functions into the parse and plugin-* packages because we didn't have a good way to share the implementations.

I ran into an issue with a model that uses parentheses in a variable name (and it's not easy to work around it in the mdl itself), so I would like to update our regex to handle this case (I will file a separate issue for that). But before fixing that issue, I would like to remove all the duplicate implementations so that we only have one place to fix.

@chrispcampbell
Copy link
Contributor Author

The parse package now exports the centralized functions:

  • canonicalId
  • canonicalVarId
  • canonicalFunctionId

I added unit tests to cover these and tweaked the implementation slightly, but should have no impact on behavior.

In the compile package, I updated the existing functions to delegate to the ones from the parse package, but didn't change their names to avoid touching too many callers (we can do that later in another pass):

  • canonicalName delegates to canonicalId
  • canonicalVensimName delegates to canonicalVarId

In the build package, I added a canonicalVarId method to the BuildContext class, which delegates to to canonicalVarId from the parse package. This way it is accessible to the plugins.

In the plugin packages, I removed the duplicate implementations and updated callers to use BuildContext.canonicalVarId.

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