-
Notifications
You must be signed in to change notification settings - Fork 513
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
Remove know-ification of elements due to post state #1819
Conversation
Why is it surprising, exactly? |
I just had a look at the documentation again and it's actually only surprising for
Searching the documentation, I see the It does seem pointless to say "be pedantic", but then have to say "no, I actually mean it". Just let me know if you want me to expand the scope of this PR to effectively make |
Oh, it can be surprising with EDIT: If you read this section for example. |
Hmm.. my only worry at this point is how much people may have based their workflow around the current behavior. |
I understand the reservation of making such a change. Especially as I tend to be that person with a weird workflow that depends on certain behaviour/implementation details. If the behaviour isn't changed, the man page really should be. It states that strict/pedantic will cause warnings/errors when entities aren't declared, without noting pending/cleared post state renders them declared (and why would it be expected that it does? You can mis-type an account just as easily when adding a pending/cleared posting as an uncleared one!). This might be why the behaviour is surprising new users. For what it's worth, I'd make I mean, it would appear the current behaviour is so unintuitive that no-one has pointed out that |
I was confused when I first encountered this behavior, and since I discovered I'm in favor of what @CandyAngel suggests, making the default be to ignore the cleared/pending/uncleared status. If there are users who rely on that functionality, I think it'd make more sense to have an option to enable it, rather than an option to disable it. I likewise agree that if the behavior isn't changed, the documentation should be updated, to make things, ahem, explicit. |
Do you want a follow-up PR to emit a message about the changed behaviour when using the involved options and another to remove |
That would be great, @CandyAngel, thank you! |
Didn't |
We definitely need a change to the manual, the man page and |
Sorry for commenting on this so late. There is one subtle feature about this option which got lost. In the past, This was a source of confusion for some... why doesn't But you could also argue that it was a feature: it was a way to let ledger warn about account names but not commodities (just don't define any commodities). Personally, I think it's fine for this to go away... it removes the confusion. But we should also acknowledge that it removes a feature that some people may have relied upon. (But imho if you define accounts, just define commodities as well... there aren't that many anyway. Payees is a different question, but that requires a separate option to be checked anyway). Example:
Old ledger: no warnings
Old ledger: warns about accounts, but not commodities |
Both those options imply that the strict/pedantic-ness is global. It could be split if people only care about certain aspects e.g. This would also mean that the options could become more consistent, by adding Could even keep the current old behaviour available and have Actually, the entities to be strict/pedantic about could be arguments to the (again, if this behaviour is desired, just let me know and I'll see if I can implement it!) With regards to |
Personally I think that's way too many new options. Like I said, I don't think it makes a great deal for accounts and commodities anyway. It makes a difference for payees but there we have an option already. However, I think it would be worth to describe the change on the mailing list (along with the subtle changes I mention) and ask if anyone has a problem with it. Overall I think the new behaviour makes more sense.
Since it's a no-op there is imho no need to remove it ever. Just remove it from the docs. Keep the option so you don't break anything. |
@CandyAngel we still need to document this. |
While it seems intentional, elements (accounts, commodities, payees and tags) becoming "known" because they happen to be in a non-uncleared posting is very surprising (and undocumented?) behaviour. It seems to be not very useful and rather unfitting with the terms "strict" and "pedantic".
This PR removes this "know-ification" and updates the tests for strict and pedantic to match their newly expected output.