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

Improve SIA experience as based on feedback #2264

Merged
merged 12 commits into from
Aug 22, 2022
Merged

Improve SIA experience as based on feedback #2264

merged 12 commits into from
Aug 22, 2022

Conversation

DidierRLopes
Copy link
Collaborator

@DidierRLopes DidierRLopes commented Aug 7, 2022

@DidierRLopes DidierRLopes added the enhancement Enhancement label Aug 7, 2022
@DidierRLopes
Copy link
Collaborator Author

@Chavithra can you help me with the tests here?

@JerBouma
Copy link
Contributor

JerBouma commented Aug 8, 2022

Could you add in somewhere (e.g. within the progress bar description) that the user can cancel the process with CTRL + C? I am pretty sure this is not common knowledge (since it also means copy).

@piiq
Copy link
Contributor

piiq commented Aug 8, 2022

Could you add in somewhere (e.g. within the progress bar description) that the user can cancel the process with CTRL + C? I am pretty sure this is not common knowledge (since it also means copy).

Won't that kill the terminal process @JerBouma

@JerBouma
Copy link
Contributor

JerBouma commented Aug 8, 2022

Could you add in somewhere (e.g. within the progress bar description) that the user can cancel the process with CTRL + C? I am pretty sure this is not common knowledge (since it also means copy).

Won't that kill the terminal process @JerBouma

The whole point of this PR is to make an exception on that right?

@piiq
Copy link
Contributor

piiq commented Aug 8, 2022

IIUC no, it's to disable default Sector/Industry

@JerBouma
Copy link
Contributor

JerBouma commented Aug 8, 2022

@DidierRLopes literary links to my issue right? #2201

And we got this:

        if ns_parser:
            try:
                (
                    self.stocks_data,
                    self.tickers,
                ) = financedatabase_view.display_bars_financials(
                    self.metric_yf_keys[ns_parser.metric][0],
                    self.metric_yf_keys[ns_parser.metric][1],
                    self.country,
                    self.sector,
                    self.industry,
                    self.mktcap,
                    self.exclude_exchanges,
                    ns_parser.limit,
                    ns_parser.export,
                    ns_parser.raw,
                    self.stocks_data,
                )
            except KeyboardInterrupt:
                pass

@piiq
Copy link
Contributor

piiq commented Aug 8, 2022

my brain is very smooth after this weekend

Copy link
Contributor

@JerBouma JerBouma left a comment

Choose a reason for hiding this comment

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

@DidierRLopes Can you include a message either within the TQDM or somewhere else that it is clear that CTRL + C cancels the process? This is not common knowledge.

@DidierRLopes
Copy link
Collaborator Author

@DidierRLopes Can you include a message either within the TQDM or somewhere else that it is clear that CTRL + C cancels the process? This is not common knowledge.

I had done that, must have forgotten to push the changes 🤦🏽‍♂️ on it

@DidierRLopes DidierRLopes merged commit f14fdfe into main Aug 22, 2022
@DidierRLopes DidierRLopes deleted the sia branch August 22, 2022 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[IMPROVE] Instead of exiting the terminal with CTRL + C, let it intercept the activated command
4 participants