Skip to content

Edit README - #219

Closed
merlin-esser-frequenz wants to merge 5 commits into
frequenz-floss:v0.x.xfrom
merlin-esser-frequenz:v0.x.x
Closed

Edit README#219
merlin-esser-frequenz wants to merge 5 commits into
frequenz-floss:v0.x.xfrom
merlin-esser-frequenz:v0.x.x

Conversation

@merlin-esser-frequenz

Copy link
Copy Markdown

Add more details for setting up and using the API

Add detailed description

Signed-off-by: merlin-esser-frequenz <116643299+merlin-esser-frequenz@users.noreply.github.com>
add more detailed description to the README

Signed-off-by: merlin-esser-frequenz <116643299+merlin-esser-frequenz@users.noreply.github.com>
add more details to README

Signed-off-by: merlin-esser-frequenz <116643299+merlin-esser-frequenz@users.noreply.github.com>
minor change in README

Signed-off-by: merlin-esser-frequenz <116643299+merlin-esser-frequenz@users.noreply.github.com>
minor change in README

Signed-off-by: merlin-esser-frequenz <116643299+merlin-esser-frequenz@users.noreply.github.com>
@github-actions github-actions Bot added the part:docs Affects the documentation label Aug 19, 2025

@matthias-wende-frequenz matthias-wende-frequenz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm unsure if add specific use cases to the readme is what we want. You should also squash your commits or give each of them a meaningful commit message.

Comment thread README.md

Please also refer to source of the [CLI tool](https://github.com/frequenz-floss/frequenz-client-reporting-python/blob/v0.x.x/src/frequenz/client/reporting/cli/__main__.py)
for a practical example of how to use the client.
### 1. Install VS Code

@matthias-wende-frequenz matthias-wende-frequenz Aug 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we shouldn't advertise specific editors in this library.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agreed.

Comment thread README.md

#### Linux / macOS

```bash

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general I like the idea of having copy paste friendly instructions. Maybe we should only focus on the reporting part in this case and skip the notebook part. I also suggest to create a separate manual page for dealing with notebooks and keep the README at a bare minimum.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I agreee with @matthias-wende-frequenz

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also this applies to most if not all projects, so probably something to be discussed here:

@cwasicki cwasicki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think these recommendations should go into the reporting client README, because they are 1) optional recommendations not required for the reporting client to run, 2) not specific to the reporting client but potentially useful for other python clients or our python tools in general.

Comment thread README.md

Please also refer to source of the [CLI tool](https://github.com/frequenz-floss/frequenz-client-reporting-python/blob/v0.x.x/src/frequenz/client/reporting/cli/__main__.py)
for a practical example of how to use the client.
### 1. Install VS Code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is not related to the reporting API, but a general recommendation how to set up a working environment. If we want to document this, I suggest to put it into another place than the reporting client README and link to it from here.

Apart from that, this sounds like vscode is required for the reporting API. It is not, and IMO it's overkill to recommend it.

Comment thread README.md

These enable Python development and interactive notebooks within VS Code.

### 2. Set Up a Virtual Environment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same for this section, not only relevant for reporting API. Do we have instructions on environments already somewhere documented @llucax ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The problem with documenting it here is that we will have to maintain similar sections in all client repositories.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Now we have https://github.com/frequenz-floss/docs/, so we can put common docs here and leave only project-specific stuff for the project. I think we still need some time to sort this out properly, but I have a rough idea of how we should move forward.

Comment thread README.md

# Install python-dotenv
# --> Used to load environment variables from a `.env` file into the projects environment
pip install python-dotenv

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be optional

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And I would remove it, we shouldn't be so opinionated on how users should run stuff. We could provide some separate guide for a newbie with some opinionated suggested configuration and IDE, but that should be definitely not part of the README.

Comment thread README.md

# Install ipkernel
# --> Python execution backend for Jupyter
pip install ipkernel

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also optional. Moreover I think there is a typo in the package name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't even know what this is 😆

Comment thread README.md
pip install ipkernel

# Register the virtual environment as a kernel
python -m ipykernel install --user --name=myenv --display-name "Python (myenv)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All of these things are optional and not required for the reporting API.

Comment thread README.md

```bash
# .env
REPORTING_API_AUTH_KEY=EtQurK8LXA8vmEd4M6DqxeNp

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I hope this is not what it looks like.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can be relieved it's not an active key!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please remove the key no matter if valid or not and replace with just a template parameter like . Surely, you can find a suggestion from the GPTs of this world ;-)

Comment thread README.md
from dotenv import load_dotenv
from datetime import datetime, timedelta
import os
import pandas as pd

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why this import?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

to use pandas data frames

Comment thread README.md
VERSION=0.18.0
pip install frequenz-client-reporting==$VERSION
# .env
REPORTING_API_AUTH_KEY=

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably good to follow our standard naming REPORTING_API_{URL,KEY,SECRET}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes, please.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@cwasicki this is copied from the current README

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we introduced signing keys we are trying to use these names as standard:

  • XXX_API_URL
  • XXX_API_AUTH_KEY
  • XXX_API_SIGN_SECRET

as using plain KEY and SECRET could be ambiguous.

@merlin-esser-frequenz

Copy link
Copy Markdown
Author

Thanks for the review!

From what I read, I realized, that I didnt really understand, what the README is supposed to be used for.
From my thinking, I wanted to collect a bunch of information for Customers, who are not that familiar with things like that, and give them a point to start.

So I guess a pdf where I summarize these steps and actively highlight, that these are just suggestions to start and that the API can be used in a lot of different other ways, makes definetly more sense.

After talking to the Customer, I realized, that they are only little steps away from really using the API to create their own notebooks, and for that I wanted to provide some help.

@matthias-wende-frequenz

Copy link
Copy Markdown
Contributor

So I guess a pdf where I summarize these steps and actively highlight, that these are just suggestions to start and that the API can be used in a lot of different other ways, makes definetly more sense.

After talking to the Customer, I realized, that they are only little steps away from really using the API to create their own notebooks, and for that I wanted to provide some help.

This repository has an auto rendered documentation which is in sync with the code. That's the place, where all of the above can be placed, for instance as tutorials or simple or advanced usage examples.

https://frequenz-floss.github.io/frequenz-client-reporting-python/

Comment thread README.md

# Activate the environment
source .venv/bin/activate # when using bash
source .venv/bin/activate.fish # when using fish

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should stick with one example environment and let users with more niche configuration figure things out for themselves (i.e. skip fish and mention at the start of the readme that we assume bash as the shell), otherwise we need to maintain all possible user configuration in our docs, which doesn't make a lot of sense.

Again something for:

Comment thread README.md
pip install pandas

# Deactivate the environment
deactivate

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why deactivating the env? If users do this then nothing works.

Comment thread README.md
deactivate
```

#### Windows (PowerShell)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We are not officially supporting Windows yet, so I wouldn't include a section on how to install in a platform we don't officially support. Maybe we should start supporting Windows, but that's a separate discussion. Again, all of this is pretty general, so we probably should include it in docs for all projects.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the context of making this a separate tutorial for newbies, we could even leave this section with a big warning saying that Windows is not YET officially supported.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Well it might discourage some people start trying to use it on Windows and hence they might solve some issues for us on the way.

@llucax llucax left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All in all, I think this is useful, but it is definitely not for a README.

BUT if you split what you created and add it to docs/tutorials/basic.md, then I guess we get a nice tutorial for newbies.

Comment thread README.md

To access the API, a key has to be created via Kuiper and stored locally on your computer. For that go through the following steps:

### 1. create .env file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is also opinionated, creating an .env file is not necessary, there are many other ways to define an env var.

Comment thread README.md
VERSION=0.18.0
pip install frequenz-client-reporting==$VERSION
# .env
REPORTING_API_AUTH_KEY=

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we introduced signing keys we are trying to use these names as standard:

  • XXX_API_URL
  • XXX_API_AUTH_KEY
  • XXX_API_SIGN_SECRET

as using plain KEY and SECRET could be ambiguous.

@llucax

llucax commented Aug 25, 2025

Copy link
Copy Markdown
Contributor

Here is an example on how to convert this into a tutorial:

@llucax llucax mentioned this pull request Sep 10, 2025
@llucax

llucax commented Sep 11, 2025

Copy link
Copy Markdown
Contributor

Closing in favor of #220.

@llucax llucax closed this Sep 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:docs Affects the documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants