Skip to content
This repository was archived by the owner on Jun 4, 2024. It is now read-only.

also allow 'parentheses' and 'percent' #617

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Conversation

theavey
Copy link

@theavey theavey commented Oct 8, 2019

For format specifiers, I had two little issues when trying to create a new table:

'parantheses' is spelled incorrectly;
also allowing 'percent' is simple enough and can save three characters!

Marc-André Rivet and others added 2 commits October 8, 2019 12:37
'parantheses' is spelled incorrectly; 
also allowing 'percent' is simple enough and can save three characters!
@theavey
Copy link
Author

theavey commented Oct 9, 2019

I can add a comment in the changelog, but this seems a little insignificant. Let me know if you think it's warranted.

If the old spelling is removed later, that would definitely be worth a comment in the changelog, and possibly a deprecation warning beforehand.

@theavey
Copy link
Author

theavey commented Oct 11, 2019

I haven't looked into the testing in this package, but I could also work on some tests for these changes.

@alexcjohnson
Copy link
Collaborator

Thanks @theavey - good catch on parantheses. @Marc-Andre-Rivet how would you like to go about removing the misspelling? Only way I can think of to produce a deprecation notice when someone uses the misspelled one is to give it a fake value in the namedtuple and then look for that fake value in the scheme method, emit a warning, and convert to the real value.

I like the shorter percent, but I'm a little hesitant to add synonyms. @Marc-Andre-Rivet do you have an opinion here?

We're using the dev branch now for active development, master is just for releases. Would you retarget your PR to dev? I'm guessing that's why the tests didn't run...

@alexcjohnson
Copy link
Collaborator

... or maybe outside PRs won't trigger CI runs in this repo? I notice #614 also didn't trigger CI. @Marc-Andre-Rivet is that the case? Can we enable that?

@theavey theavey changed the base branch from master to dev October 17, 2019 13:00
@theavey
Copy link
Author

theavey commented Oct 17, 2019

The "percent" was less important to me, but it was the word I intuitively put in before looking up the correct keyword. I understand hesitating to put in synonyms for clarity and to avoid bloat.

I retargeted the PR, but that seems to have generated the merge conflict because I started from the master branch.

@Marc-Andre-Rivet
Copy link
Contributor

or maybe outside PRs won't trigger CI runs in this repo?

They didn't. That's changed now. The next commit should trigger CI.


parantheses / parentheses: Yes! Let's definitely fix this and tag the misspelled one for deprecation in v2.x + create an issue for follow up.

percent / percentage: Also hesitant to add synonyms or to rename. Since no one seems to feel strongly about it, would drop it from the PR. Could be tagged alongside the typo for renaming in v2.x, it does feel a bit more consistent with the money format.

@Marc-Andre-Rivet
Copy link
Contributor

Marc-Andre-Rivet commented Nov 29, 2019

@theavey Took the liberty of merging changes from dev and updating the .gitignore file. The build artifacts will need to be removed from the PR.

That said, thanks for sticking with it, this PR has been waiting for too long. 🙇

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

Successfully merging this pull request may close these issues.

3 participants