On Thu, Nov 29, 2018 at 10:10 AM Daniele Nicolodi <[email protected]>
wrote:
> Hello Maetin,
>
> On 29/11/2018 07:08, Martin Blais wrote:
> > Let me get this right: you're proposing a patch that affects the parser
> > layer, that has potential to break a bunch other people's stuff, and
> > you're not bothering to write a test that specifically handles that case?
> > This is a perfect example how not to contribute to a released open
> > source project.
> > I have no time for "largely untested" patches.
>
> I think I didn't explain well enough what my idea is (however, if you
> would have looked at the patch I believe you could have understood it
> rather quickly).
>
I had looked at it actually.
The problem with adding support for this (i.e., changing the sort order
rule) is that there will be a lot of other things that start relying on it.
So say, you change the schema to implicitly support date or datetime in the
printer, as your patch suggests.
Now some codes use date, some codes use datetime.
Someone uses the result of some importer code that puts datetime in this
field - code that they didn't write, say - and passes it to some routine
which fails on datetime. Boom! A bug. (Note that date's don't have a
timezone and aren't subject to some of the timezone related issues.) Or say
that person does arithmetic on the dates/datetimes. And it fails. That's
unpredictable behavior. Then I'll get a request to fix that bug on an
unsupported data type and someone will be surprised it's not supported
officially.
Or someone comes to rely on the particular ordering given by the datetime
and then I refine the rules in a way that breaks their code.
Or... well, the ordering rules specify that balance check are to be sorted
such that they're located before other types of directives (see link I
sent). What about a Balance check with a time after a transaction
directives on the same day?
That's bad. The schema needs to have a single specific, well-documented
type. Python is already loose enough. The schema needs to be clear. I even
considered switching to protocol buffers at some point in order to enforce
correct datatypes, it's already bad enough these aren't checked.
So I took one look at your patch and I thought all of this.
(We've discussed this issue numerous times on the mailing-list.)
The right solution would be to commit to using datetime everywhere in the
source code and make that the unique new datatype (probably with a bunch of
instances with time = midnight). But that's not going to happen in this
major version, that would be for a future major revamp. This current
release is a stable release and people need it to work, it's not a place to
experiment so much at the moment, this needs to remain stable, a future
revamp would be the right venue for that, and it would come with many other
changes. Plus, it's really much more complicated that it seems (timezones?).
I've agreed with others in previous threads to add
1. Support for the grammar to parse a time and to insert the time portion
in some metadata field
2. Adding an option to support specifying a secondary key for sorting
This wouldn't break any of the current code by default.
>
> The problem I tried to solve is that in the ingestion process, before
> being serialized into a beancount file, entries are sorted by date,
> type, and origin file line number. It has ben requested if the sorting
> order could take into account transaction time.
>
> The solution I proposed is to allow the ingestion machinery to accept a
> datetime object for the date field. The time would be thrown out in the
> serialization, but would be used for sorting. It is similar to using
> time information stored in the meta field.
>
> The implementation is simple as the only thing that needs to change is
> the serialization in minimal part. After looking at the code the only
> thing that needs to be done is to replace the string formatting for date
> entries from ...format('{e,date} ...', e) to
> ...format('{e.date:%Y-%m-%d}...', e). There is no change to the parser
> layer.
>
> The patch does not come with tests because I thought to it as a better
> description of the kind of solution I'm proposing, rather than something
> ready to be merges.
>
> I interpret your answer as very hostile (please correct me if this
> impression is wrong). Thus I am happy that I haven't invested more time
> in my attempt to contribute an idea to your project.
>
> Best,
> Dan
>
> --
> You received this message because you are subscribed to the Google Groups
> "Beancount" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to [email protected].
> To post to this group, send email to [email protected].
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/beancount/f9097208-7347-1532-fcdb-5986dce3518e%40grinta.net
> .
> For more options, visit https://groups.google.com/d/optout.
>
--
You received this message because you are subscribed to the Google Groups
"Beancount" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/beancount/CAK21%2BhPvgdT8YMQ_CLZF_3na5Die%2BWSN6N2j0YsN%2BOZ9sAmUAQ%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.
