Skip to content

Conversation

@benoit-cty
Copy link
Contributor

@benoit-cty benoit-cty commented Mar 28, 2025

The ApiClient class is creating a run in init. But when we are using it only to get data it create useless entry in the database.

And now that the new API is in production, we could remove the warning.

@benoit-cty benoit-cty requested a review from inimaz March 28, 2025 20:33
@benoit-cty benoit-cty marked this pull request as ready for review April 2, 2025 16:47
Copy link
Collaborator

@inimaz inimaz left a comment

Choose a reason for hiding this comment

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

Thanks @benoit-cty! Maybe we can remove the call to self._create_run() in the __init__? I see that we create the run anyhow if not present in the add_emissions See here/

@benoit-cty
Copy link
Contributor Author

Thanks @benoit-cty! Maybe we can remove the call to self._create_run() in the __init__? I see that we create the run anyhow if not present in the add_emissions See here/

Well, I think not having a run_id in add_emissions is more a workaround in case of a network error.

I prefer the explicit way of not asking to add a run.

@benoit-cty benoit-cty requested a review from inimaz April 14, 2025 18:40
Copy link
Collaborator

@inimaz inimaz left a comment

Choose a reason for hiding this comment

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

Thanks @benoit-cty ! With this, the API client will be in better shape. I mentioned a minor comment

Copy link
Member

@SaboniAmine SaboniAmine left a comment

Choose a reason for hiding this comment

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

Thanks!

@benoit-cty benoit-cty merged commit a7d78ef into master Apr 15, 2025
4 checks passed
@benoit-cty benoit-cty deleted the feat/api_cleaning branch April 15, 2025 08:52
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.

4 participants