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

Portfolio menu bug fixes #3204

Merged
merged 51 commits into from
Nov 9, 2022
Merged

Portfolio menu bug fixes #3204

merged 51 commits into from
Nov 9, 2022

Conversation

montezdesousa
Copy link
Contributor

@montezdesousa montezdesousa commented Oct 31, 2022

Description

Fix issues mentioned here #3072, #3069, #3068

  • This PR fixes [Bug] portfolio/bench - no autocompletion available for flag -b or --benchmark #3069
  • Change orderbook references to transactions
  • change load progress to something closer to tqdm for consistency with remaining terminal
  • remove line between show and bench commands
  • Fix Warning Please first define the portfolio (via 'load') and the benchmark (via 'bench').
  • Fix var calculation
  • var and es table values should be changed to % 2 decimals format
  • Remove autocompletion for numbers that can take any value
  • summary values are missing % symbol
  • default sum to True in holdv and holdp. Remove total value from legend. See [IMPROVE] Redesign portfolio/holdv and portfolio/holdp #3121
  • alloc assets -t tables have no spacing between them
  • Message: Loading country/region data: shows up out of nowhere with alloc -a countries command
  • Message: Loading sector data: shows up out of nowhere with alloc sectors
  • mret --raw outputs 2 tables, one for benchmark, other for portfolio. Should be identified which is which in the title.
  • remove limitation of having negative risk free rates

Checklist:

Others

  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.

@montezdesousa montezdesousa added bug Fix bug portfolio Portfolio menu labels Oct 31, 2022
@JerBouma
Copy link
Contributor

Could you make sure that the rfr, as mentioned here #3074, is an actual percentage? E.g. 0.1 = 0.1%. It is undesirable you need to enter 0.001 to get 0.1% (and can be confusing to work with).

@montezdesousa montezdesousa marked this pull request as ready for review November 1, 2022 14:44
Copy link
Contributor

@hjoaquim hjoaquim left a comment

Choose a reason for hiding this comment

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

In #3072 you mentioned:

Pull Metrics commands to before Graphs because this is what gives you a snapshot of your portfolio
Is this something to drop?

I've added this small commit that I think goes more according to the guidelines (only one empty line after the user input and before output, and also just 1 empty line separating each table).
Let me know if you agree with this.

Other than this, I think everything described is addressed and issues are gone.
No other remarks, very nice job on this!

@montezdesousa
Copy link
Contributor Author

In #3072 you mentioned:

Pull Metrics commands to before Graphs because this is what gives you a snapshot of your portfolio
Is this something to drop?

I've added this small commit that I think goes more according to the guidelines (only one empty line after the user input and before output, and also just 1 empty line separating each table). Let me know if you agree with this.

Other than this, I think everything described is addressed and issues are gone. No other remarks, very nice job on this!

Thanks, yes I drop that one, bc I found it a bit subjective after all so I let it stay as it was. Looks good to me! Tks

@montezdesousa montezdesousa merged commit f9086d6 into main Nov 9, 2022
@montezdesousa montezdesousa deleted the portfolio_qa branch November 9, 2022 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fix bug portfolio Portfolio menu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants