exact (multicommodity, total) assertions #902
Conversation
ag-eitilt
force-pushed
the
exact-assertion
branch
from
d85931e to
f957530
Compare
| char '=' | ||
| pure sourcepos | ||
| sourcepos <- genericSourcePos <$> lift getSourcePos | ||
| char '=' |
There was a problem hiding this comment.
Having a hard time understanding this commit.. is it incomplete ? Message needs clarification.
There was a problem hiding this comment.
Mostly, it doesn't change anything about the behaviour, just the code structure. These particular lines of code move the skipping of spaces and the optional call out to JournalReader.hs, to better reflect that the parse function relates to the assertion itself rather than encoding surrounding file structure as well (which brings it in line with some other parsers in the file). More generally, that commit just changes BalanceAssertion from being a typedef'ed tuple to being a standalone datatype.
Yeah, that could probably be merged with behavioural changes, but this way the foundation is separate from the exact assertions; I could create a new branch off of this commit adding sub-account-inclusive assertions without anything related to commodity-exclusive assertions.
There was a problem hiding this comment.
Actually, which message were you referring to? I'm happy enough to try to clarify that, but I'm too close to the code to realize which one seems confusing from the outside.
hledger-lib/Hledger/Data/Types.hs
Outdated
| baposition :: GenericSourcePos | ||
| } deriving (Eq,Typeable,Data,Generic,Show) | ||
|
|
||
| instance NFData BalanceAssertion | ||
|
|
||
| data AssertionFlags = AssertionFlags { | ||
| afexact :: Bool |
There was a problem hiding this comment.
Let's add some docs, eg -- ^ Does this assertion check the full multi-commodity balance or only one commodity ?
There was a problem hiding this comment.
Also for BalanceAssertion, eg -- | A statement about the expected account balance after a particular posting.
There was a problem hiding this comment.
PS I'd be fine with inlining the flags in BalanceAssertion, your call.
There was a problem hiding this comment.
Good catch. The Types module in general could use better docs, but that's no reason to compound the issue. And I'm not entirely sure why I wasn't inlining the flags to begin with, thanks!
simonmichael
added
journal
ag-eitilt
force-pushed
the
exact-assertion
branch
from
719fee7 to
2f98a1a
Compare
|
Actually, which message were you referring to? I'm happy enough to try to clarify that, but I'm too close to the code to realize which one seems confusing from the outside.
I meant the commit message. Thanks for your comments, I'll respond later.
|
ag-eitilt
force-pushed
the
exact-assertion
branch
3 times, most recently
from
9052257 to
ea2ec38
Compare
|
I've lost track of this a bit. Can I ask for some review from others ? Go/no-go for merge ? |
simonmichael
added
needs:value-proposition
|
For reference, here's my summary of the earlier PR, with related discussion below it: A discussion I've lost makes me think this also changes to be a multicommodity assignment, so a's dollar balance is set to 1 but now also any other commodities it may contain are set to 0. This seems like a change to existing semantics, unlike the rest of this PR (IIRC). |
|
(PS and this part is not in the proposed docs yet, unless I've misplaced it.) |
I misspoke, I think it doesn't change that. It allows new multicommodity/exact assignments: |
|
Yeah, if it does change |
.gitignore: products of manual cabal builds, and vim temp files hledger.cabal: *.RootFinding removed from statistics >=0.15 in favor of that in math-functions >=0.2
ag-eitilt
force-pushed
the
exact-assertion
branch
from
a94d353 to
4e8731b
Compare
|
Sorry for the slow response. Getting ready to merge this. I will combine "lib: Changes shared between expressions/exact assertions" (2f9db90) into the next commit for clarity. |
simonmichael
removed
needs:impact-analysis
|
Merged, thank you very much! (I renamed the PR to "multi-commodity assertions" for posterity, I prefer that slightly so as not to suggest that exactness/accuracy is exceptional for us :) |
simonmichael
changed the title
|
Still chewing on what we should call these. "multicommodity" could still be ambiguous it (you could imagine a multicommodity assertion that still allows additional unasserted commodities). Complete assertions ? Total assertions ? |
More focused PR derived from #871, containing only the changes required to allow assertions over the entire contents of an account. Written primarily as groundwork and proof-of-concept for #556.