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

Fix : Improve readability #212

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

PhRX
Copy link
Contributor

@PhRX PhRX commented Apr 12, 2017

Improved readability by adding options for thousands separators, choosing decimal separators etc.

Replaces deleted PR #194 .

Add preferences dialog for formatting the displayed and exported
numbers, with choices for thousands and decimal separators etc.
Ensure the preferences are respected in all rendered fields.
@PhRX
Copy link
Contributor Author

PhRX commented Apr 12, 2017

Commit 58dd0f5 reverts all previous commits from 13de748 up to and including 1e1e31e, so please ignore those commits in the PR commit list.

@RichardWarburton
Copy link
Member

This is still a very large PR - could it be split up somehow?

@PhRX
Copy link
Contributor Author

PhRX commented May 3, 2017

@RichardWarburton I reviewed the PR, but much like #210 I don't see any way to prise apart the changes.

As in #210 there's only one logical split I can see - separating out the formatting logic (i.e. all methods for printing a number in a particular format) from the configuration logic (selecting the actual format). But since the second obviously depends on the first, again I do not see how I can create 2 independent branches out of this.

Any advice would be greatly appreciated.

@RichardWarburton
Copy link
Member

I think splitting out the formatting logic and configuration logic changes would be a good idea. I mean this is still a 1700 line diff PR in it's current form.

Btw - my apologies for the delays in getting back to you on these PRs. I've found it very difficult to allocate time to this project until recently.

@PhRX
Copy link
Contributor Author

PhRX commented Aug 28, 2017

I have absolutely no headroom for looking at this at the moment, I'll try and get to it ASAP.

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.

2 participants