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

Add min parameter #39

Merged
merged 10 commits into from
Sep 18, 2023
Merged

Add min parameter #39

merged 10 commits into from
Sep 18, 2023

Conversation

s-simoncelli
Copy link
Contributor

No description provided.

Copy link
Member

@jetuk jetuk left a comment

Choose a reason for hiding this comment

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

This looks good to me. A couple of suggestions if you have time:

  • Rebase this branch against master.
  • Implement: impl TryFromV1Parameter<MinParameterV1> for MinParameter.

We should add docstrings to the schemas as well, but I need to agree a format for these first.

@s-simoncelli
Copy link
Contributor Author

I have opened a new pull request on the schema project. Once that's approved I'll implement TryFromV1Parameter.

Let me know when you've decided abotu the docstring and I'll update the code. Do I need to add a docstring to the parameter as well?

@jetuk
Copy link
Member

jetuk commented Sep 7, 2023

I have opened a new pull request on the schema project. Once that's approved I'll implement TryFromV1Parameter.

Thanks I've merged this.

Let me know when you've decided abotu the docstring and I'll update the code. Do I need to add a docstring to the parameter as well?

I think we should probably just follow this format: https://doc.rust-lang.org/rust-by-example/meta/doc.html

@s-simoncelli
Copy link
Contributor Author

I added the docstring to the schema, let me know if it's OK.

I also need to pull the new changes from the schema repository, but there is no new version. Can you please push a new tag so that I can update the Cargo.toml file and implement the parameter conversion?

Cheers

@jetuk
Copy link
Member

jetuk commented Sep 18, 2023

I'll merge #45 and then merge or rebase this branch against main.

@jetuk jetuk merged commit 5f9702c into pywr:main Sep 18, 2023
1 check passed
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