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

Add manual color selection to stocks #3

Closed ConradSelig closed 3 years ago

ConradSelig commented 3 years ago

This is something that was implemented before #2 (re-architecture), so looking back at this commit will help re-implement this feature: 7373b50eddf0b19030c19646b6b1c6020e231664

Color selection should be available by both stocks_config and by cli arguments.

ConradSelig commented 3 years ago

@yonMaor I was thinking of tackling this ticket myself but thought you might be interested in making this your next issue.

Should be fairly straightforward - basically all of the code you need is already in the commit mentioned in the opening comment. Just look at the code in that commit and re-implement the feature back into the new architecture.

Let me know sometime this week if you're interested / what questions you have. Otherwise I'll tackle it next weekend.

yonMaor commented 3 years ago

@ConradSelig I can take a look at it later this week (might only get to it towards the weekend). Not exactly sure what I need to do after a very quick look, but I'll check it in more depth, and let you know if I have any specific questions.

ConradSelig commented 3 years ago

No rush on this - not a super high priority item, just a really nice quality of life feature.

The most important lines in that commit for you will be in main.py lines 153-160. This is where I was converting the users given color name to a hex value that plotille accepted. That conditional block is also hard to read, so treat it as:

if the user did not provide a color:
          pick an automatic color and draw the graph
else if a manual color is given:
          convert the given color from name to hex
          draw the plot with the converted color

I expect plenty of questions, so don't hesitate to reach out :+1:

yonMaor commented 3 years ago

@ConradSelig Finally got around to this issue. How and where do you want to ask the user what color they want to use for each stock?

Edit: nevermind I think I misunderstood the issue. Working on it now, will let you know if I have any questions

yonMaor commented 3 years ago

I tried running the current code, but I'm getting a division by zero error in line 464, not sure if it's the new code, or something I'm doing wrong though

ConradSelig commented 3 years ago

Everything on the main branch currently works for me - likely something you've added then.

yonMaor commented 3 years ago

I think the issue is with my portfolio.ini file. Is there supposed to be some kind of header, or does it jump straight into stock data like in the readme?

yonMaor commented 3 years ago

Also, the script can be run without a portfolio.ini file, which is why I was getting the division by zero error

ConradSelig commented 3 years ago

Hmm interesting - I thought there was a check for that. I'll add a ticket for it. Did you figure out the format for portfolio.ini? There is a complete format example of that config file in the README.

yonMaor commented 3 years ago

Haven't figured it out yet. When using the format from the readme file, I get the following error:

MissingSectionHeaderError: File contains no section headers. file: 'portfolio.ini', line: 1 'HPE:\n'

Also, considering the problems that I've run into, it might be a good idea to add an example of a portfolio file that the user can base their portfolio on

ConradSelig commented 3 years ago

Good plan. I'll upload an example portfolio.ini for you right now. Give me a couple minutes.

yonMaor commented 3 years ago

Another question, in the code you linked, when the user did not specify the color, it was chosen with the following code: lc=webcolors.hex_to_rgb(auto_colors[i % 67])

Can you explain the i % 67 part?

ConradSelig commented 3 years ago

Here is the commit with the example portfolio.ini for you.

There are 67 hex values saved for auto selection. This list can still be seen in autocolors.py. This particular list of hex colors was picked because each color is visually distinct from each other color. This was done so that if 67 different graphs were plotted all on one axis - the user would still be able to distinguish one stock to the next. But what if the user has 68 stocks to graph? We don't have any more colors to use - so we loop back to the start. By using the modulo operator we can easily loop back to the start of the list. 68%67 -> 1 69%67 -> 2 35245%67 -> 18 (even with over 35 THOUSAND stocks, we still get a valid color index!)

Hopefully that clears it up for ya!

yonMaor commented 3 years ago

Two things I'm noticing in the original code (checked because it was happening with my code and I wanted to make sure that it wasn't a bug that I had added):

  1. I'm not seeing any colors on the graphs (although the text after the graphs is colored).
  2. Stocks are printed on different plots whether the independent_graph parameter is set to True or False in the configuration file.
ConradSelig commented 3 years ago

I'm not surprised - before #2 the whole program was a mess. Def don't just copy all of the old code (most of it doesn't work anyways). Just use it for reference.

yonMaor commented 3 years ago

I meant the code that is currenty in the main branch. Sorry if I was unclear.

ConradSelig commented 3 years ago

Ah I see - sure enough. Looks like independent graphs is always on regardless of set options. I feel like I just fixed this bug yesterday... apparently I am not doing enough testing.

Color seems to be working OK for me however - even with multiple graphs.

ConradSelig commented 3 years ago

@yonMaor make another pull if you feel the need (just make sure not to overwrite your changes) - just pushed a commit to fix the independent_graphs option(s) not working.

Still not sure what your color problem is... mine looks fine. Screenshot from 2021-02-27 02-44-29

yonMaor commented 3 years ago

It's not showing any colors on my end. Weird. I noticed that I'm using pytohn 3.8.5 could that be the issue?

ConradSelig commented 3 years ago

Possibly - could also be a dependencies problem.

yonMaor commented 3 years ago

Well, while I try to figure this out, as far as I know, the user currently can't input color (the color set for each graph are from the autoclor file, and there's no option in the portfolio for a color for each stock currently).

Is that addition also required here, or do you want to solve that with a seperate issue?

ConradSelig commented 3 years ago

That IS the problem this ticket is for - autocolor for multiple graphs is already working.

Manual color selection is what I'm looking for now.

yonMaor commented 3 years ago

@ConradSelig So I think that I'm pretty much done. Two important factors to consider:

  1. I still can't see the colors in the terminal for some reason, so I tested the feature by checking the color that the gen_graph function recieved, and it looks ok on my end.
  2. Currently the user chooses the color in the portfolio.ini file for each stock, with colors from the autocolor file (with hex code). If you want some other format for the color selection please let me know
ConradSelig commented 3 years ago

@yonMaor - sorry for the long wait, it's been a crazy week for me and I havn't had time for this project.

I love the hex selection! Ultimate customization even I hadn't thought off... I'm still really looking for "by name" color selection. i.e. the user can set the color to "light blue".

Again the code you should need to convert color names to hex values is in the commit first referenced.

yonMaor commented 3 years ago

@ConradSelig no worries, I was working on other things in the meantime :) If we want "by name" color selection, I do need a list of color names (I remember that we had one in the code and readme, but it was removed at some point). Otherwise, I can try to translate the list from autocolors to names

ConradSelig commented 3 years ago

Same list of colors from the old README! You can use this line of code to convert a color name to hex:

hex_color = webcolors.CSS3_NAMES_TO_HEX[user color]

Obviously replace user color with wherever you are storing the user color.

yonMaor commented 3 years ago

Ok so to summarize the things that I've added to create the feature:

  1. Manual color selection to the portfolio.ini file (added instructions in the readme). This is either from the approved list, or using Hex code (which gives the used complete freedom with the color, as you mentioned).
  2. Color selection when actually drawing the graph (one of the three cases for no manual color, worded manual color, and hex manual color).
  3. Input test in the Portfolio.populate function, where I check that the color entered is valid.

I hope that my code isn't messy as this feature was a little more complex than the previous one. If you have any comments, please let me know once I've commited it.

ConradSelig commented 3 years ago

Done and done! Another ticket knocked out.

Impressive work @yonMaor - glad you're on the team. Can't wait to find out which ticket you want to tackle next.

yonMaor commented 3 years ago

Glad to be here :) I'm somewhat new in the programming world, and very new in open source, so all feedback is great to have (both positive and negative). If you have anything specific that you wan me to take on next please let me know. Otherwise I'll just look at the open issues and find something new