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

separate options parsing for byteTrace and visitTrace functions #226

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

alexzurbonsen
Copy link

@alexzurbonsen alexzurbonsen commented Sep 7, 2024

Triage

Type of change

Please select any of the below items that are appropriate.

This pull request:

  • Adds a new carbon estimation model to CO2.js
  • Adds new functionality to an existing model
  • Fixes a bug with an existing model implementation
  • Add other new functionality to CO2.js
  • Add new data to CO2.js
  • Improves developer experience for contributors
  • Adds contributors to CO2.js
  • Does something not specified above

Related issue/s

Please list below any issues this pull request is related to.

Docs changes required

Do any changes made in this pull request require parts of the CO2.js documentation to be updated?

  • Yes
  • No
  • I don't know

If you answered "Yes", please create an corresponding issue in our Developer Documentation repository.

Describe the changes made in this pull request

Context: see #244

Summary:
This PR splits options parsing into two separate functions, one for the perByteTrace function and one for the perVisitTrace function such that only the options relevant to the function are parsed.

Alternatives: considered: introduce a flag parameter in the existing function to return early when the function is called from perByteTrace. To me this seems less clear, but it would be less invasive.
Also one could create another parsing function for the additional visit options, but not sure that is desirable.

@alexzurbonsen
Copy link
Author

I have made the change, but I cannot install the project locally. There is a build failure in one of the dependencies that I do not have the time to figure out. Let's see what the ci says.

@alexzurbonsen
Copy link
Author

I didn't understand yet, why the unit tests are failing. The computed values are always a bit off. Not sure if I made a mistake in the option parsing or if some other inputs evolved. but not sure why that should affect versioned older builds.

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.

1 participant