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

Public export of getExecutableRealPath and refactoring loading the exe path #34

Merged
merged 6 commits into from
Jan 20, 2024

Conversation

FLuzzi-csw
Copy link

Export to the public the call to getExecutableRealPath since it can be usefull,
also added the sibling calls GetExecutableDefaultOldPath and GetExecutableNewPath.

The function LoadPath is sync.Once to limit performance related issues

Copy link

@Bluebugs Bluebugs left a comment

Choose a reason for hiding this comment

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

Just a small change request, looks good otherwise and thanks for the tests.

executable.go Outdated Show resolved Hide resolved
@FLuzzi-csw
Copy link
Author

FLuzzi-csw commented Jan 17, 2024

The ci is failing while downloading github.com/mattn/goveralls
https://github.com/fynelabs/selfupdate/actions/runs/7545086229/job/20540137088

It seems that GO111MODULE=off is the problem, fixed in e12750b

@Bluebugs
Copy link

The ci is failing while downloading github.com/mattn/goveralls https://github.com/fynelabs/selfupdate/actions/runs/7545086229/job/20540137088

It seems that GO111MODULE=off is the problem, fixed in e12750b

Yeah, I am just running the test again. If it doesn't pass, the way it could work is by bumping the minimal version of go to 1.19 (and max to 1.21) along with using go install github.com/mattn/goveralls@latest instead of go get github.com/mattn/goveralls

@Bluebugs
Copy link

It actually passed. Well, will do a different PR for upgrading the version.

@Bluebugs Bluebugs merged commit 24aee1e into fynelabs:main Jan 20, 2024
4 checks passed
@andydotxyz
Copy link

Shouldn't the "LoadPath method be private? It doesn't seem to perform any public function.

@FLuzzi-csw
Copy link
Author

FLuzzi-csw commented Jan 22, 2024

I thought of LoadPath as a fail fast route if you need to call multiple times GetExecutableRealPath, GetExecutableDefaultOldPath and GetExecutableNewPath (i've applied this logic in the function selfupdate.apply).

Maybe I could have written the docs to be more clear on it's usage or make LoadPath a must call before GetExecutableRealPath & Co, even though that seems excessive to me.

Let me know if you need any further clarifications or modifications

@andydotxyz
Copy link

That doesn't seem to explain why it should be public. It runs the code just once and is always executed before any of the getters return. Why expose it at all?

@FLuzzi-csw
Copy link
Author

FLuzzi-csw commented Jan 22, 2024

To give an a different and clear escape hatch if something goes wrong, making all the following calls to the getters "safe",
without forcing you to call it at the start of the program. (that seems a nightmare)

That said, if this is not seen as useful or maybe decremental, I can make LoadPath private.

@andydotxyz
Copy link

making all the following calls to the getters "safe", without forcing you to call it at the start of the program.

This is what I am not understanding. What is unsafe about not calling the LoadPath manually? None of the test code calls it.

@FLuzzi-csw
Copy link
Author

Got the lightbulb moment, that is just redundant at best and confusing at worst.

Tell me if you want to fix it, otherwise I'll make a PR later in the day.
Thanks for the feedback

@andydotxyz
Copy link

If you are happy to fix this with another PR that would be excellent thanks.

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.

3 participants