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

Ported fmt.Errorf() -> fmt.Println() #15

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

IAmRiteshKoushik
Copy link
Collaborator

@IAmRiteshKoushik IAmRiteshKoushik commented Nov 15, 2024

Change Log

  • Removes all fmt.Errorf in favour of fmt.Println (better fix needed)
  • Formats the entire repository with go fmt

This PR completes issue #6

Please review and merge @Ashrockzzz2003 @Abhinav-ark

Copy link
Member

@Ashrockzzz2003 Ashrockzzz2003 left a comment

Choose a reason for hiding this comment

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

While we now actually are making use of those lines. It now raises a new problem.

The problem

Consider the case where the user did not use Amrita VPN and an error is thrown. What happens is:

Execute() -> start() -> getCoursesReq() -> fetchHTML()

In fetchHTML()

fmt.Println("Error fetching the URL. Make sure you're connected to Amrita WiFi or VPN.")

This line prints the error first.

But we're also carrying the err string from fetchHTML back all the way to start() and printing it.

Now in the output: - TWO ERROR messages but in reverse order.

Error fetching the URL. Make sure you're connected to Amrita WiFi or VPN.
failed to fetch the HTML content

This doesn't look good to me. If we can give a cause and effect relationship to this and print properly, it will be useful.
Else, we can just ignore this error message.

Or have a logger that separates debug and info level error messages to make better use of the logs.

What's your take on this @IAmRiteshKoushik and @Abhinav-ark?

@IAmRiteshKoushik
Copy link
Collaborator Author

IAmRiteshKoushik commented Nov 16, 2024

@Ashrockzzz2003 if we are porting the application to a TUI then we can have a diagnostics box on the right hand side of the screen where things can be monitored constantly and error messages / success messages can be printed.

It can keep track of all events that are happening within the application and give visual updates to the user. (think of it as being similar to the chat section of a multiplayer gaming lobby - PLAYER JOINED, PLAYED LEFT, etc.)

@Abhinav-ark
Copy link
Member

@Ashrockzzz2003 @IAmRiteshKoushik ErrorF was used because I wanted the application to stop in case of Errors.

@Abhinav-ark
Copy link
Member

While we now actually are making use of those lines. It now raises a new problem.

The problem

Consider the case where the user did not use Amrita VPN and an error is thrown. What happens is:

Execute() -> start() -> getCoursesReq() -> fetchHTML()

In fetchHTML()

fmt.Println("Error fetching the URL. Make sure you're connected to Amrita WiFi or VPN.")

This line prints the error first.
But we're also carrying the err string from fetchHTML back all the way to start() and printing it.

Now in the output: - TWO ERROR messages but in reverse order.

Error fetching the URL. Make sure you're connected to Amrita WiFi or VPN.
failed to fetch the HTML content

This doesn't look good to me. If we can give a cause and effect relationship to this and print properly, it will be useful. Else, we can just ignore this error message.

Or have a logger that separates debug and info level error messages to make better use of the logs.

What's your take on this @IAmRiteshKoushik and @Abhinav-ark?

I faced this issue when i tried to change Errorf to print.

@Ashrockzzz2003
Copy link
Member

@Ashrockzzz2003 @IAmRiteshKoushik ErrorF was used because I wanted the application to stop in case of Errors.

But return is already there to stop. But I'm sure Errorf did not stop execution, return did.
Let's try TUI
See how it goes.

@Ashrockzzz2003
Copy link
Member

@Ashrockzzz2003 if we are porting the application to a TUI then we can have a diagnostics box on the right hand side of the screen where things can be monitored constantly and error messages / success messages can be printed.

It can keep track of all events that are happening within the application and give visual updates to the user. (think of it as being similar to the chat section of a multiplayer gaming lobby - PLAYER JOINED, PLAYED LEFT, etc.)

let's try

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Remove unused line or make it useful. Unused returns from fmt.Errorf() across entire project
3 participants