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

make myip command not cache by default #114

Merged
merged 3 commits into from
Jul 20, 2022
Merged

make myip command not cache by default #114

merged 3 commits into from
Jul 20, 2022

Conversation

K3das
Copy link
Contributor

@K3das K3das commented Jul 19, 2022

Solves #88

--cache flag will enable the cache if someone wishes to enable it.

- M

Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

I wouldn't remove the flag, that breaks backwards compatibility and someone may be using it in some script somewhere to disable the cache already.

We can just introduce a new flag --cache which defaults to false and make the cache behave such that:

  • --cache is off and --nocache is off, then disable cache.
  • --cache is off and --nocache is on, then disable cache.
  • --cache is on and --nocache is on, then disable cache.
  • --cache is on and --nocache is off, then enable cache.

So fNoCache = fNoCache || !fCache.

@K3das
Copy link
Contributor Author

K3das commented Jul 19, 2022

Considerably simpler solution was just to have --nocache default to true, and --nocache=false would enable the cache again. I don't know why I didn't realize this yesterday...

Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

Yeah you're right, I actually also considered it but didn't know assigning it to false explicitly would work, but I guess it would (I think - did you test that?).

ipinfo/cmd_myip.go Outdated Show resolved Hide resolved
@K3das
Copy link
Contributor Author

K3das commented Jul 19, 2022

Yup, I tested it...

@UmanShahzad UmanShahzad merged commit 4d8df50 into ipinfo:master Jul 20, 2022
@UmanShahzad
Copy link
Contributor

Will release it a bit later grouped with some more changes. Thanks a ton!

@rm-Umar rm-Umar mentioned this pull request Aug 10, 2022
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

2 participants