tobi-wan-kenobi / bumblebee-status

bumblebee-status is a modular, theme-able status line generator for the i3 window manager.
https://bumblebee-status.readthedocs.io/en/main/
MIT License
1.22k stars 228 forks source link

Updated contrib/publicip module and util/location #897

Closed tfwiii closed 2 years ago

tfwiii commented 2 years ago

Added another API endpoint, Added options to display country name, country code, city name and lat/long coordinates, attempt to handle failure to fetch info from API endpoints cleanly

P.S. This is my first ever pull request so apologies if I have got anything wrong!

tobi-wan-kenobi commented 2 years ago

Thanks a lot, the PR looks great!

I added a few minor details, could you please address those?

Also, would it maybe be easier to just add a format string as config option? That would allow the user to specify format as well as present elements, and would probably shorten the code substantially, as well.

What do you think about that?

tfwiii commented 2 years ago

Thanks a lot, the PR looks great!

I added a few minor details, could you please address those?

Also, would it maybe be easier to just add a format string as config option? That would allow the user to specify format as well as present elements, and would probably shorten the code substantially, as well.

What do you think about that?

Glad it's useful!

Sorry to be dim but this is the first pull request I've ever done so I'm a little bit lost!

How do I view your comments?!

Wrt using a format string as a config option - happy to look at this. Are there any other modules that take this approach that I can use as a guide?

Thanks!

tobi-wan-kenobi commented 2 years ago

That is an excellent question! I have actually never seen this process from a PR creator perspective. I think you should see my comments if you go to the "Changes" section of the PR (the diff), the comments are inlined there.

Reg. the format: The most useful example is the "disk" module, I believe, you can find a full list by going to https://bumblebee-status.readthedocs.io/en/main/modules.html and searching for ".format"

Thank you for your contribution and patience!

tfwiii commented 2 years ago

That is an excellent question! I have actually never seen this process from a PR creator perspective. I think you should see my comments if you go to the "Changes" section of the PR (the diff), the comments are inlined there.

Reg. the format: The most useful example is the "disk" module, I believe, you can find a full list by going to https://bumblebee-status.readthedocs.io/en/main/modules.html and searching for ".format"

Thank you for your contribution and patience!

I'm sorry, I never found your comments!

But I've updated using the format string approach for parameters which I think is a big improvement.

Please let me know what you think :)

tobi-wan-kenobi commented 2 years ago

Looks really good, thanks!

Just 2 requests (that I originally put in the vanished comments):

  1. Please do not change the default interval - I think 1h is reasonable, given that public IPs, countries, etc. do not change frequently. Also, the interval can be changed configuratively.

  2. Please do not change the location API (country vs country name) - I think this might be used in different modules already.

Going forward, it might make sense to also add a call to the location API to retrieve a dict of values, so that you don't have to maintain them manually and individually in the module. For now, it looks really good, though.

As soon as 1 and 2 are addressed, I will merge the PR.

Many thanks for your work on this!

tfwiii commented 2 years ago

Looks really good, thanks!

Just 2 requests (that I originally put in the vanished comments):

1. Please do not change the default interval - I think 1h is reasonable, given that public IPs, countries, etc. do not change frequently. Also, the interval can be changed configuratively.

2. Please do not change the location API (country vs country name) - I think this might be used in different modules already.

Going forward, it might make sense to also add a call to the location API to retrieve a dict of values, so that you don't have to maintain them manually and individually in the module. For now, it looks really good, though.

As soon as 1 and 2 are addressed, I will merge the PR.

Many thanks for your work on this!

Done!

(I think/hope :) )

tobi-wan-kenobi commented 2 years ago

Looks good, many thanks!