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

Big refactor and various improvements #39

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Big refactor and various improvements #39

wants to merge 6 commits into from

Conversation

reaganmcf
Copy link
Owner

Since I haven't worked on this project in a while, I have learned a lot about Rust in the meantime. Going back over this codebase I see lots of bad practices and better ways to do things.

However, I'm not changing any variable names yet or any architectural changes, just refactoring where it is trivial. Also, I am removing logging since we don't need anything that intense, and print statements are more than enough for this application.

Lastly, this patch also is meant to close #1, since I did implement colored output and better output messages. Below is a screenshot
Screen Shot 2021-07-19 at 10 52 27 PM

@reaganmcf reaganmcf added the enhancement New feature or request label Jul 20, 2021
@reaganmcf reaganmcf added this to the Version 1.0 milestone Jul 20, 2021
@reaganmcf reaganmcf self-assigned this Jul 20, 2021
@reaganmcf reaganmcf marked this pull request as ready for review July 22, 2021 01:18
@reaganmcf reaganmcf requested a review from alayshahh July 22, 2021 01:29
@reaganmcf
Copy link
Owner Author

@alayshahh No idea why the windows stuff isn't working - going to fix that later. But, if you could review the rest of the patch that would be appreciated 👍🏻

Copy link
Collaborator

@alayshahh alayshahh left a comment

Choose a reason for hiding this comment

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

couple of unused imports and etc. can clean up later on, once refactor is complete. Colored with windows does not work due to some weird thing w Power Shell/Command Line and using ANSI (I remember this being a big issue when I was working on it).
https://stackoverflow.com/questions/51680709/colored-text-output-in-powershell-console-using-ansi-vt100-codes

@reaganmcf
Copy link
Owner Author

I'm gonna take a look through https://github.com/nushell/nushell to see how they do it. They support all platforms and get colored output with no issues, by maintaining a modified version of ansi_term called nu_ansi_term

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make output messages more user friendly and informative
2 participants