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

prettify examples: add main demo screen to select specific examples #117

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

Conversation

elv-gilles
Copy link

This patch allows to compile all examples in one binary.

A main 'demo' example is added on top allowing to run each of the specific examples - although each example can still be run individually. see the added README

@mjarkk
Copy link
Member

mjarkk commented Mar 8, 2022

image

The screen is not fully visible on a small terminal.

Copy link
Member

@mjarkk mjarkk left a comment

Choose a reason for hiding this comment

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

I think we should use go run . in the examples to keep it simpeler

_examples/README.md Outdated Show resolved Hide resolved
_examples/main.go Outdated Show resolved Hide resolved
* use 'go run .' in usage and text message
* make 'main' view sensitive to screen dimensions
@dankox
Copy link

dankox commented Mar 9, 2022

I'm just wondering, why did you use all the _ assignement in the code? It's kinda incosistent as for example the quit() functions, somewhere assigns the g and v to _ and somewhere it's right in the function definition.
Also I see a bunch of Fprintf and other function calls with these types of assignements.

I don't really understand the reason to put these assignements to a function where you don't check the return value, so I'm wondering what was your reason to add them there.
I know for sure that for example Coverity makes a complain if you don't check returned error values (like in case of the Fprintf), but I don't think there is any coverity running in the pipeline, so there shouldn't be problems with it.
So if you could provide the reason, that would be nice. Not that the code is incorrect, I am just confused by that.

@elv-gilles
Copy link
Author

I'm just wondering, why did you use all the _ assignement in the code?
Not that the code is incorrect, I am just confused by that.

Humm, sorry to be confusing.
I'm running my dev environment with warning enabled for unused variables and unhandled errors which I find very useful.
... but if folks in the project don't like it, I'll refrain myself.

@elv-gilles elv-gilles requested a review from mjarkk March 9, 2022 20:00
@dankox
Copy link

dankox commented Mar 10, 2022

Ok... so let's wait for @mjarkk input.

@mjarkk
Copy link
Member

mjarkk commented Mar 11, 2022

I personally don't like the _ assignments but that's just personal preference.

Tough that said they are also out of the scope of this PR so let's not add them.

@elv-gilles
Copy link
Author

@mjarkk - I fixed the github workflow and therefore need another approval.

@elv-gilles
Copy link
Author

@dankox @mjarkk - is there anything else I can do ? I don't have the power to merge this myself.

@mjarkk mjarkk requested a review from dankox April 2, 2022 12:55
@dankox
Copy link

dankox commented Jun 16, 2022

I haven't been around for some time and now I see that this is not really merged.

So I will just give here my opinion, but @mjarkk feel free to merge.

I can see that most of the _ were removed from the original code, but I can still see them in the "main" program.
The reason why I don't really like it is because this is only for the sake of squiggly lines in the IDE. It doesn't really add anything to the code, or to its safety. It only makes it look a bit messy.
If the linter is complaining about such code, it should be fixed by properly checking the error, instead of just putting there _. Because this doesn't really resolve the problem the linter is reporting. It just hides warning message.
It's not really a good practise because the problem is still there and people can't find it out easily anymore. Just wanted to say it so it's clear why I don't like it and complain about it :) These are just demo examples, so no biggie ;) but could be harmful in some other bigger apps if used like this.

As for the PR. At first I was a bit hesitant as I like to have possibility to run just one program using go run without any of the select menu thingy when testing something.
But this actually allows such run too, so that's good enough for me and will make it easier for people to quickly try different demos. So 👍 :)

@elv-gilles
Copy link
Author

Hello @dankox @mjarkk can we merge this, please ?

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

Successfully merging this pull request may close these issues.

None yet

3 participants