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

Add some documentation #6

Open
snth opened this issue Oct 8, 2017 · 10 comments
Open

Add some documentation #6

snth opened this issue Oct 8, 2017 · 10 comments

Comments

@snth
Copy link
Owner

snth commented Oct 8, 2017

In particular I would like to describe how to use the ~/.coinrc file.

What is the modern way of easily adding documentation? I would prefer to write in markdown rather than rst.

@MichaelCurrin
Copy link
Contributor

Yes I agree mark down is widely used and a good one use. I see you use both formats at the moment.

I have plenty of experience with SafeConfigParser, as a variation of ConfigParser from the same configparser library. So I can write instructions on how to use it. I imagine it's a file created by hand like this.

[Luno]
api_key_id: 123456
api_key_secret: abcdef

It could grow with more exchanges. What else are you planning to use the file for?

@snth
Copy link
Owner Author

snth commented Oct 11, 2017

Exactly.

i was thinking that we could also put Assets and Currencies in there and for different exchanges.

So from Luno I only want to get BTC/ZAR and from Bitfinex BTC,ETH in USD and EUR.

So it would try .coinrc -> environment variable -> command line option.

@MichaelCurrin
Copy link
Contributor

Ah yes that all makes sense.

Oh and I see you also have ~/.config/numismatic.ini read in at the top of cli.py - do you want that documented in instructions too?

@MichaelCurrin
Copy link
Contributor

I have improved the handling of the config file for the Luno keys (such as using config.has_section()

And I actually implemented something on listen to get assets and currencies specific to exchange from the config file. I don't know if anyone was working on it but anyway I have working well with priority when comparing with environment variables and arguments, so I can get that in soon.

I'll write the documentation next and then docs and implementation in together.

Here is what my .config file looks like.

# Numismatic configuration file.

[Bitfinex]
assets: ETC,ATL
currencies: USD 

[Luno]
# Luno.com API keys.
api_key_id: ca7ma...
api_key_secret: 6mOY........................................

@MichaelCurrin
Copy link
Contributor

Are you fine with the instructions to go in as a file called docs/config_instructions.md unless you have a better name? The file starts like this.

Configuration Instructions

Set values for assets, currencies and API keys for `coin` to read.

These are the possible approaches:

1. Command-line arguments
2. Environment variables
3. Config file
4. System defaults

I've implemented a config file approach for listen but I need to test the other functions like info and prices to make sure they handle environment variables. Currently I'm getting an error setting environment variables for prices, to do with joining and splitting I think.

@barrysteyn
Copy link
Contributor

@MichaelCurrin @snth
Michael, before you document, I think that the way we are doing config right now is quite complex. Rather than have two config files (numismatic.ini and .coinrc) as well as environment variables, I would like to suggest we just one config file (lets use .coinrc). The config parse can very easily allow for environment variables within that file, and so it won't be needed for the cli decorator.

This will be much simpler, both to code against and to document. What do you guys say?

@MichaelCurrin
Copy link
Contributor

@barrysteyn Sure. I'm not using or documenting the numismatic.ini file as thought it was unnecessary.

Are you saying leave out the environment variables feature? That could be good to simplify code and instructions, both now and maintaining in future. But up to @snth.

On the config file, I noticed that we can use exchange name as heading for setting assets and currencies for listen -e luno collect run. But, for something like prices, we don't have an exchange name as section, so can't set assets and currencies, unless there is a section in the config called [prices] or [global]. What do you think?
Currently environment variable can be used to set that, as

$ NUMISMATIC_CURRENCIES='ZAR'
$ coin prices

@barrysteyn
Copy link
Contributor

@MichaelCurrin Yes, seeing as we can incorporate env variables inside the .coinrc, my suggestion is to leave it out all together. Makes the code easier to maintain, makes documentation easier as well. @snth - you have the final say, but what we could do is commit a config file that uses env variables for the secrets (like the API key) which allow it to work out the box in a container that could expose the variables, and it would also allow anyone to easily overwrite if they don't want to use env variables.

@snth
Copy link
Owner Author

snth commented Oct 16, 2017

Thanks for your comments.

@MichaelCurrin I think exchange specific asset and currency options would be awesome. I'm keen to see how you have set this up.

@MichaelCurrin @barrysteyn I think ConfigParser and click both provide ways for using a config setting or an environment variable. I haven't looked at these in detail so whatever is easiest. I'm happy for you guys to suggest something.

@MichaelCurrin I think just submit your Pull Request and then we can discuss the different options for pulling in the environment variables. Your code doesn't have to be ready to merge yet. We can just use it as a base for discussion so that we are not discussing in a vacuum.

Sorry about the numismatic.ini confusion. I was experimenting and should have deleted it again. Will do so now.

@MichaelCurrin
Copy link
Contributor

@barrysteyn @snth Ok great. I've added my changes from the other day to a feature branch on my fork and did a tempory pull request to my own master, so you can see the changes. MichaelCurrin#1 Hopefully you can see enough from the diff there.
There are conflicts when I merge the original repo into my fork, so I need time to apply those then add my changes around. And of course will consider what we discussing here so will probably drop the feature branch and do something fresh.

Yes I find having a config file to view and edit a lot easier than managing what is in stored in export and don't see a need to have both. A config file is also persistent and accessible across shell sessions, which I like.

Yes I agree that a versioned config file as a template with some base values would be good and it could be used to replaced the global defaults at the top of cli.py file. (This also makes it easier for a user to see what the default config options are)

The approach I'm used to at work is as follows and may suitable here too:

  • repo/app/etc/app.conf - versioned file with defaults settings. This file should never be updated by users. (It can be updated by commits to get changes/corrections out). It should not contain any sensitive credentials - these could be blank and the application could raise and error or ignore them, depending on desired behaviour. e.g. the app can prompt the user to complete details in app.local.conf because keys are missing or invalid.
  • repo/app/etc/app.local.conf - optional unversioned file created by hand by the user (this can be in setup instructions). When creating the config object in python, we always read app.conf first and then app.local.conf, where the 2nd file will overwrite any values in the first and the 2nd one could be a few sections or all the sections of the 1st. The location of this file could be in the $HOME directory if you prefer?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants