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

Move json getter extensions to core crate #286

Merged
merged 12 commits into from
Feb 19, 2025

Conversation

iantei
Copy link
Collaborator

@iantei iantei commented Feb 13, 2025

This PR is the implementation for #88

  1. Moved the following from routee-compass crate to routee-compass-core crate:
    • compass_configuration_error.rs
    • compass_configuration_field.rs
    • config_json_extension.rs
  2. Updated the use from routee-compass to routee-compass-core for the above CompassConfigurationError, CompassConfigurationField and ConfigJsonExtensions
  3. Introduced new error type - CompassComponentError which includes PluginError - removed PluginError from CompassConfigurationError since we Plugins is specifically in routee-compass.
  4. OutputPluginBuilder utilizes the CompassComponentError, since it needs PluginError.

@iantei iantei marked this pull request as ready for review February 13, 2025 17:37
Copy link
Collaborator

@robfitzgerald robfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks @iantei for taking care of this, we have had to use a lot of work-arounds to do similar JSON->Rust object build methods elsewhere in the workspace. We should be able to start unraveling that and also standardize these extension methods as well, since there is some redundancy. 🍻

@robfitzgerald
Copy link
Collaborator

@iantei this PR has been approved. we usually leave clicking "Merge pull request" to the PR author. if you're not given that option, let me know, maybe our GitHub repo settings aren't correctly set.

@iantei iantei merged commit 42a55a0 into NREL:main Feb 19, 2025
5 checks passed
@iantei
Copy link
Collaborator Author

iantei commented Feb 19, 2025

@robfitzgerald I thought the review was still pending from @nreinicke. Thank you for letting me know about the process.

@robfitzgerald
Copy link
Collaborator

I thought the review was still pending from @nreinicke

no problem! on this repo, we only require one approval from a write-authorized contributor to merge a PR, unless someone says otherwise (like if the PR is complex and could use a second review).

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

Successfully merging this pull request may close these issues.

2 participants