-
Notifications
You must be signed in to change notification settings - Fork 12
Edit README #219
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
Edit README #219
Conversation
Add detailed description Signed-off-by: merlin-esser-frequenz <[email protected]>
add more detailed description to the README Signed-off-by: merlin-esser-frequenz <[email protected]>
add more details to README Signed-off-by: merlin-esser-frequenz <[email protected]>
minor change in README Signed-off-by: merlin-esser-frequenz <[email protected]>
minor change in README Signed-off-by: merlin-esser-frequenz <[email protected]>
There was a problem hiding this comment.
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.
|
||
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed.
|
||
#### Linux / macOS | ||
|
||
```bash |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
|
||
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 |
There was a problem hiding this comment.
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.
|
||
These enable Python development and interactive notebooks within VS Code. | ||
|
||
### 2. Set Up a Virtual Environment |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
||
# Install python-dotenv | ||
# --> Used to load environment variables from a `.env` file into the projects environment | ||
pip install python-dotenv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be optional
There was a problem hiding this comment.
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.
|
||
# Install ipkernel | ||
# --> Python execution backend for Jupyter | ||
pip install ipkernel |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😆
pip install ipkernel | ||
|
||
# Register the virtual environment as a kernel | ||
python -m ipykernel install --user --name=myenv --display-name "Python (myenv)" |
There was a problem hiding this comment.
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.
|
||
```bash | ||
# .env | ||
REPORTING_API_AUTH_KEY=EtQurK8LXA8vmEd4M6DqxeNp |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 ;-)
from dotenv import load_dotenv | ||
from datetime import datetime, timedelta | ||
import os | ||
import pandas as pd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this import?
There was a problem hiding this comment.
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
VERSION=0.18.0 | ||
pip install frequenz-client-reporting==$VERSION | ||
# .env | ||
REPORTING_API_AUTH_KEY= |
There was a problem hiding this comment.
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}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Thanks for the review! From what I read, I realized, that I didnt really understand, what the README is supposed to be used for. 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/ |
|
||
# Activate the environment | ||
source .venv/bin/activate # when using bash | ||
source .venv/bin/activate.fish # when using fish |
There was a problem hiding this comment.
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:
pip install pandas | ||
|
||
# Deactivate the environment | ||
deactivate |
There was a problem hiding this comment.
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.
deactivate | ||
``` | ||
|
||
#### Windows (PowerShell) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
||
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 |
There was a problem hiding this comment.
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.
VERSION=0.18.0 | ||
pip install frequenz-client-reporting==$VERSION | ||
# .env | ||
REPORTING_API_AUTH_KEY= |
There was a problem hiding this comment.
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.
Here is an example on how to convert this into a tutorial: |
Closing in favor of #220. |
Add more details for setting up and using the API