ConradSelig / cliStocksTracker

A command line utility for tracking a stock market portfolio. Primarily featuring high resolution braille graphs.
Apache License 2.0
55 stars 14 forks source link

Command line configuration / configuration overhaul #12

Closed ConradSelig closed 3 years ago

ConradSelig commented 3 years ago

Any arguments given as part of the command line arguments should override any settings given in the config files.

Due to the nature of how many arguments will be available. I will not be accepting any pull requests that do not have detailed documentation included.

lemonase commented 3 years ago

I am going to start working on taking CLI arguments. I think the config is probably the main barrier to getting started with this project at the moment.

It would be nice if there was an example config along with documentation. This could be generated dynamically in the code, or by simply adding the file.

What do you think about consolidating everything into one standardized config file? configparser is in the python standard library, and it parses .INI like files. There's also libraries for parsing YAML, TOML and JSON if we want to do that.

Let me know if you want to make another issue for this or just make this the main spot for all config-related work.

ConradSelig commented 3 years ago

Let's just make this the home for all config related issues.

I don't want command line arguments to be required - but they should supplement / override any existing config file (I do like a consolidated config file). Config file should come pre-packaged with a default config file, which includes all options currently in 'config' (because those are required).

I like the .ini format, partially because it already is a similar match to what we have, partially because you suggested it by name and I'm assuming you already have some experience with it.

That would make the config file look like this:

[frame]
width=60
height=20

[kwargs]
independent_graphs=True
timezone=America/New_York
rounding_mode=down

; theoretical way to insert stocks into the config, this would also help with the problem in #5 
[stock symbol]
graph=True
; my light research shows that it is in fact possible to handle duplicate keys 
; https://stackoverflow.com/questions/30095471/configparser-duplicate-keys-in-python
buy=10@46.12
buy=40.765@47.65
sell=14@48.00

From what I can tell by my 30 second Google search on the .ini format - every key needs a "section", so we can't just leave the kwargs options hanging. Looks like convention would have that section named "General" however. @lemonase thoughts? I'm still assuming you know more than I do in this regard.


Also the problem on internal config handling. While I agree a single external config file is preferable - I'm actually still partial to separating the program settings from the stock settings. That would still leave us with two instances of the Configuration class. This should simplify the code by only passing around the data we need for each case. I'll yield to you @lemonase if you feel strongly otherwise.

Let me know what all you want to cover on this ticket @lemonase and I'll take the rest (or none at all, your call!).

ConradSelig commented 3 years ago

We should include one stock in the default config file for users who want to edit the config file by hand instead of inserting / buying / selling stocks only with the command line options.

lemonase commented 3 years ago

I honestly don't have that much experience implementing these config libraries (yet!), but I do configure lots of different programs.

configparser is nice because its builtin and seems simple, but YAML/TOML are also massively popular and probably more versatile (I think they're closer to JSON). They might be more complex to implement... I'll have to do more research on that.

I don't mind having two distinct config files either (one for stock data, one for program settings). It "breaks conventions", but I think its justified here because one is like the main data you're feeding into the program and the other is settings.

I'm going to be working on adding command line arguments for now though.

ConradSelig commented 3 years ago

Great - I'll overhaul the config files tonight. I've done YAML before - so I'm actually going to shift away from that and use this as an opportunity to learn something new.

Not sure what I'll pick just yet - something that works easily with duplicate keys (again for #5) for sure.

ConradSelig commented 3 years ago

Decided to go with the .ini format - It's very similar to what I originally did which made the conversion over to the new format basically effortless.

'config' is now 'config.ini'
     'kwargs' is now 'General'
     'frame' is now 'Frame'
'stocks_config' is now 'portfolio.ini'

Here is my current config.ini for reference: config.ini.txt

I haven't done as much testing or error catching as I want to just yet - but I'm out of time tonight and want @lemonase to have a chance to get a look at the changes, so I've pushed the conversion to a new branch config_overhaul.

@lemonase I see you've made a large number of changes in your fork! Can't wait to get a closer look at what you've pulled together for us. Hopefully the format switch won't make any more work for you.


As an additional sidenote - there is still no default config file that comes packaged with the project. I don't care if we have a default file in the repo or if we add a "generate config" option - but either works and we should have one.

lemonase commented 3 years ago

Nice, that's a good start! I'll wait until you merge with the main branch before doing my PR.

ConradSelig commented 3 years ago

I'm going to throw in a default config.ini file.

@lemonase Can you implement a "generate config" option regardless? Might be nice for users to be able to just start from scratch if they accidently mess up their config too much.

ConradSelig commented 3 years ago

Incredible work on this one @lemonase. +1 to you.

Thanks for the contribution! Let me know if something else you want to work on pops up.