feat(lib): co2js: allow config and globalConfig parameters as input #50

alexzurbonsen · 2024-03-09T00:41:33Z

Types of changes

Enhancement (project structure, spelling, grammar, formatting)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

A description of the changes proposed in the Pull Request

This is a suggestion on how to fix #49. There may be different approaches.

This one helps to enable old functionality within the new architecture.

If this PR finds acceptance, I would still need to adapt documentation.

alexzurbonsen · 2024-03-16T08:30:23Z

Hi if-team,

would it be possible to get some feedback on this? I guess you are quiet busy currently, so anything you need to ease the process, let me know.
Background: I am relying on this change with another plugin I am currently writing. If it is not happening I would have to create another version of the co2js plugin somewhere else.

Thank you!

jmcook1186 · 2024-03-18T09:33:00Z

Hi @alexzurbonsen sorry for the delay, we were slammed last week finalizing the repos for the hackathon and missed this. We'll chat over this today and get you some feedback asap.

alexzurbonsen · 2024-03-18T10:47:31Z

Hi @jmcook1186, thanks, that sounds good.

jmcook1186 · 2024-03-21T14:42:20Z

Hi @alexzurbonsen, thanks a lot - we looked at this and agree that having those params all in global config is not the best approach. However, the way we would prefer to resolve it is to have those values coming from inputs or defaults. This requires a moderate refactor of the plugin, but it would be more consistent with how we intend for the plugins to work. @manushak is looking over this in some more detail shortly.

manushak

Please update the README file of the plugin to reflect that the options come from the input

manushak · 2024-03-21T15:02:31Z

src/lib/co2js/index.ts


 const {InputValidationError} = ERRORS;

 export const Co2js = (globalConfig?: ConfigParams): PluginInterface => {


could you please remove the globalConfig, we don't need it anymore

manushak · 2024-03-21T15:06:13Z

src/lib/co2js/index.ts

+  /**
+   * Validates Global config parameters.
+   */
+  const validateGlobalConfig = () => {


remove this function as well, as we don't need it anymore

manushak · 2024-03-21T15:36:34Z

src/lib/co2js/index.ts

-      })
-      .partial()
+    const inputSchema = bytesSchema
+      .merge(greenWebHostSchema)


The green-web-host parameter should be obtained only from the node config

Are you sure? This would be sad, because it wouldn't support my use case. I wrote a plugin to get the green-web-host parameter separately and feed it into this one. This doesn't work if it is a config option.

We assumed this parameter was constant at the level of a node in the manifest but if that's not the case I think we can probably flex on whether it is in config or inputs. @manushak can either confirm or push back.

Also, just because there are times we don't want to merge something into our version of a plugin, doesn't mean you can't host your preferred version elsewhere. The ideal outcome for us is to have many people all maintaining version so the plugins that work for their specific use cases in many repositories, and we just maintain a minimal "standard lib" of essentials. We want the plugin ecoystem to be as decentralized as possible so we are strongly in favour of people forking and modifying our plugins even if the changes don't get upstreamed.

Hi @jmcook1186, thanks for your explanation. Makes sense to me.

My reasoning is that it is more convenient to have one standard version of a plugin that is allowing for a broad spectrum of use cases, than having several slightly modified versions of the same plugin throughout the ecosystem where people have to understand what the differences are etc. That might be confusing and a bit frustrating.
A plugin that is as general as possible makes the latter case less likely. So I think there is value in making a plugin as versatile as possible.

But if you don't want to include it, of course, I will just create a variant of it.

we can be flexible and accept the green-web-host either in node config or input

alexzurbonsen · 2024-03-27T19:55:52Z

I made most of the changes, except that I kept green-web-host as an input and config parameter fow now. If you want indeed want it to only appear in config, I can change that, but that wouldn't solve my entire problem then.

manushak

please, update the readme file as well

manushak · 2024-04-01T12:42:48Z

src/lib/co2js/index.ts

-      validateGlobalConfig()
-    );
+    const validatedConfig = validateConfig(config);
+    const mergedValidatedConfig = Object.assign({}, validatedConfig);


mergedValidatedConfig doesn't have any sense anymore

manushak · 2024-04-01T12:51:33Z

src/lib/co2js/index.ts

-      })
-      .partial()
+    const inputSchema = bytesSchema
+      .merge(greenWebHostSchema)


we can be flexible and accept the green-web-host either in node config or input

manushak · 2024-04-01T13:00:05Z

src/lib/co2js/index.ts

+  const greenWebHostSchema = z.object({
+    'green-web-host': z.boolean().optional(),
+  });
+  const optionsSchema = z.object({


move optionsSchema and bytesSchema schemas to the validateInput function

manushak · 2024-04-01T14:42:51Z

src/lib/co2js/index.ts

-   * Validates input parameters.
+   * Validates input and config parameters.
   */
  const validateInput = (input: PluginParams) => {


you can add the config as a second parameter and after merging write
.refine(() => !!config['green-web-host'] || !!input['green-web-host'], { message:Green web host is provided neither in config nor in input.\nConfig: ${config}\nInput: ${input}, })

this will allow us to get rid of validateGreenWebHostExists function

I added a second check to make sure it is not provided twice. Let me know if that is not how you want it to be.

jmcook1186 · 2024-04-17T11:54:15Z

Hi @alexzurbonsen and @manushak - thanks for your work on this, I hope it's been useful for your hackathon project Alex.
Here's my push back on this PR - we have a strong design principle that prefers to know where to expect data to come from in a manifest. I think this PR has some benefits over the original that would be nice to merge in, but I'd really prefer a strong definition for where green-web-host belongs - it makes sense to me to have it in inputs if you need it to vary across timesteps - and not in either/or of inputs/config.

Two potential routes forwards: a) we can merge the changes if green-web-host can comes from inputs only (my preference), or b) we can close this out and you maintain your version in your own repo.

Cheers

alexzurbonsen · 2024-04-17T21:24:17Z

Hi @jmcook1186, sounds good to me. I like the design principle. Thanks for clarifying. Of course, I am okay with both routes forwards. Personally, I also have a preference for having one plugin instead of two. But I think both routes are totally fine. Let me know how you decide @manushak and @jmcook1186. Then I will either close this PR or make the necessary changes.
Best, Alex

jmcook1186 · 2024-04-25T13:41:17Z

Hi @alexzurbonsen please feel free to make the proposed changes and we'll be happy to merge. Let me know if you decide otherwise and I'll close this out.

alexzurbonsen · 2024-05-17T17:20:17Z

Hi @jmcook1186, finally had time to push the changes.

jmcook1186

lgtm!
@manushak can you confirm you are happy too?

manushak · 2024-05-29T06:37:24Z

src/lib/co2js/README.md

      inputs:
        - timestamp: 2023-07-06T00:00
          duration: 1
          network/data/bytes: 1000000


please, move green-web-host: true into inputs (it is on line 69)

manushak · 2024-05-29T06:38:47Z

src/lib/co2js/README.md

          green-web-host: true
      inputs:
        - timestamp: 2023-07-06T00:00
          duration: 1


the same here, move green-web-host: true into inputs (it's on line 113)

manushak · 2024-05-29T06:39:12Z

src/lib/co2js/README.md

      outputs:
        - timestamp: 2023-07-06T00:00
          duration: 1
          network/data/bytes: 1000000


add green-web-host: true into outputs

manushak · 2024-05-29T06:48:29Z

src/lib/co2js/README.md

    {
      duration: 3600, // duration institute
      timestamp: '2021-01-01T00:00:00Z', // ISO8601 / RFC3339 timestamp
      'network/data/bytes': 1000000, // bytes transferred


move 'green-web-host': true, // true if the website is hosted on a green web host, false otherwise into inputs (it's in line 172). The same in line 193

Thanks, missed these changes.

manushak · 2024-05-29T07:09:32Z

src/lib/co2js/index.ts

      })
-      .refine(allDefined, {
-        message: '`type` and `green-web-host` are not provided in node config',
+      .refine(data => data['type'] !== undefined, {


you can remove the refine method call, it is no longer necessary

Signed-off-by: alexzurbonsen <alexander.zur.bonsen@tngtech.com>

manushak

thanks, @alexzurbonsen, for your contribution. You have done amazing work!
@jmcook1186, it's ready to merge

alexzurbonsen marked this pull request as draft March 9, 2024 02:11

alexzurbonsen marked this pull request as ready for review March 9, 2024 02:29

alexzurbonsen changed the title ~~feat(lib): allow config and globalConfig parameters as input~~ feat(lib): co2js: allow config and globalConfig parameters as input Mar 9, 2024

manushak requested changes Mar 21, 2024

View reviewed changes

alexzurbonsen force-pushed the co2js-allow-config-as-input branch from ea47532 to 5e09c70 Compare March 27, 2024 19:54

alexzurbonsen requested a review from manushak March 27, 2024 19:56

manushak requested changes Apr 1, 2024

View reviewed changes

alexzurbonsen force-pushed the co2js-allow-config-as-input branch 2 times, most recently from 58d6fc7 to b09f215 Compare April 2, 2024 23:51

alexzurbonsen requested a review from manushak April 2, 2024 23:51

alexzurbonsen force-pushed the co2js-allow-config-as-input branch from b09f215 to 5a88156 Compare April 2, 2024 23:59

manushak approved these changes Apr 5, 2024

View reviewed changes

manushak added size: medium medium sized task awaiting-qa PR is waiting for QA approval fix PR fixes a bug or improves something labels Apr 5, 2024

manushak requested a review from MariamKhalatova April 5, 2024 11:52

alexzurbonsen force-pushed the co2js-allow-config-as-input branch 2 times, most recently from 4388a60 to 4c5c06d Compare May 17, 2024 18:45

jmcook1186 approved these changes May 23, 2024

View reviewed changes

manushak reviewed May 29, 2024

View reviewed changes

feat(lib): allow config and globalConfig parameters as input

cac94b2

Signed-off-by: alexzurbonsen <alexander.zur.bonsen@tngtech.com>

alexzurbonsen force-pushed the co2js-allow-config-as-input branch from 4c5c06d to cac94b2 Compare May 31, 2024 23:52

manushak approved these changes Jun 14, 2024

View reviewed changes

jmcook1186 merged commit f4e7367 into Green-Software-Foundation:main Jun 14, 2024

alexzurbonsen mentioned this pull request Jun 23, 2024

Add docs for usage of co2js within the impact framework thegreenwebfoundation/developer-docs#59

Closed


		const {InputValidationError} = ERRORS;

		export const Co2js = (globalConfig?: ConfigParams): PluginInterface => {

feat(lib): co2js: allow config and globalConfig parameters as input #50

feat(lib): co2js: allow config and globalConfig parameters as input #50

Uh oh!

Conversation

alexzurbonsen commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Types of changes

A description of the changes proposed in the Pull Request

Uh oh!

alexzurbonsen commented Mar 16, 2024

Uh oh!

jmcook1186 commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexzurbonsen commented Mar 18, 2024

Uh oh!

jmcook1186 commented Mar 21, 2024

Uh oh!

manushak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexzurbonsen commented Mar 27, 2024

Uh oh!

manushak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmcook1186 commented Apr 17, 2024

Uh oh!

alexzurbonsen commented Apr 17, 2024

Uh oh!

jmcook1186 commented Apr 25, 2024

Uh oh!

alexzurbonsen commented May 17, 2024

Uh oh!

jmcook1186 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manushak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexzurbonsen commented Mar 9, 2024 •

edited

Loading

jmcook1186 commented Mar 18, 2024 •

edited

Loading