Skip to content

Conversation

abobov
Copy link
Contributor

@abobov abobov commented Dec 10, 2017

No description provided.

@alerque alerque self-requested a review July 1, 2019 12:49
@alerque
Copy link
Member

alerque commented Jul 1, 2019

Thanks for taking the time to contribute this @abobov. I realize it has been a long time since you posted this, but I've recently stepped in to help maintain this plugin and hope to see this integrated. That being said I have some questions about the code if you're still around...

Copy link
Member

@alerque alerque left a comment

Choose a reason for hiding this comment

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

If I understand this correctly, the default is to leave the spelling on for amounts (commodity + value). It seems to be that turning it off would actually be the more sensible default with an option to override the default and enable it for people with odd requirements (would that ever actually be useful?).

@abobov
Copy link
Contributor Author

abobov commented Jul 1, 2019

@alerque Yes, for me it is more reasonable to disable spell check by default, I just tried to keep behavior as-is by default.

@alerque
Copy link
Member

alerque commented Jul 2, 2019

Keeping default behavior in general is a good goal, but in this case I think it would be safe and advisable to go ahead and make the default be not spell checking the commodities/amounts. This reduces the load of things we expect people to do for a sane config. I think the desire for the opposite behavior will be trivial to non-existent.

Would you be willing to make the change and update this PR?

@alerque alerque merged commit 3ddfb14 into ledger:master Jul 2, 2019
@alerque
Copy link
Member

alerque commented Jul 2, 2019

Thanks again for the contribution and for being willing to touch it up for merger even years later.

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