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

currency: add thousands separator, dynamic precision, and optional HTTPS for Fixer #1988

Merged
merged 4 commits into from May 14, 2021

Conversation

Duckle29
Copy link
Contributor

Description

The current code only adds more precision when converting bitcoin. Other crypto currencies are also valued such that they return very small numbers.
This change finds the required amount of digits to display at least 2 non-zero decimals.

It also adds thousands separators to the reply.

Checklist

  • I have read CONTRIBUTING.md
  • I can and do license this contribution under the EFLv2
  • No issues are reported by make qa (runs make quality and make test)
  • I have tested the functionality of the things this change touches

One issue was reported by make qa, but in help.py (ambiguous variable name l)

Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

The code is a bit confusing at first, so it needs at least a few comments to make it easier for future reader to understand what is going on.

Furthermore, our commit convention is to use this format:

plugin-name: reason

Not [Plugin Name] reason.

sopel/modules/currency.py Outdated Show resolved Hide resolved
@dgw dgw changed the title [Currency] Added thousands separator and dynamic precision currency: add thousands separator and dynamic precision Nov 13, 2020
@dgw
Copy link
Member

dgw commented Nov 14, 2020

Merging #1989 should fix the spurious CI failures. Keep an eye on this build—if it passes, feel free to squash/rebase away.

@dgw
Copy link
Member

dgw commented Nov 28, 2020

d59d59e is pretty ridiculous. What in the world is Fixer doing with HTTPS that it costs too much to provide for all users?!

@Duckle29
Copy link
Contributor Author

image I guess it's to make this look nice and stair-case shaped. It's really dumb. I didn't have issues with this before, but API complained today :/

@dgw dgw added this to the 7.1.0 milestone May 10, 2021
@dgw dgw added the Tweak label May 10, 2021
@dgw dgw requested a review from Exirel May 10, 2021 01:18
@dgw
Copy link
Member

dgw commented May 10, 2021

I'll take care of rebasing/squashing this, since we haven't heard from @Duckle29 anywhere in a while. @Exirel What do you think of the current state, assuming I clean up the history to fit commit message conventions and remove fixups?

Exirel mentioned the possibility that an API key could contain invalid
characters, theoretically, and I figured it was worthwhile to add in a
step to make that value safe for URLs as long as I was touching this
plugin's code again.
Kind of annoying to get things like "1e06 XLM" in the output when we've
been so careful to make sure the converted value won't be printed as
engineering notation. This is a simple fix.
@dgw dgw changed the title currency: add thousands separator and dynamic precision currency: add thousands separator, dynamic precision, and optional HTTPS for Fixer May 10, 2021
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

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

Let's hope this doesn't brake break too often...

@dgw
Copy link
Member

dgw commented May 10, 2021

Brake? Even if that's a typo, I don't see what would break.

@Exirel
Copy link
Contributor

Exirel commented May 10, 2021

The Fixer API, or any other currency API. It feels like it's going downhill like the weather APIs. 😞

@dgw
Copy link
Member

dgw commented May 10, 2021

Ah, Fixer is just getting the sh*t monetized out of it. There are several other Fixer-compatible sites out there; the two we merged a few days ago are just a random selection. Still thinking about that true "custom" option, to provide one last-ditch escape hatch for 7.1.x clients with a free-entry URL config value.

@dgw dgw merged commit 2dd80d0 into sopel-irc:master May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants