Skip to content

Conversation

@jbrockmendel
Copy link
Contributor

Two main goals here:

  1. perform validation more consistently
  2. de-duplication, improved consistency.

e.g. at the moment we have 3 places where we in principal should* be setting res.ampm, but we are only actually doing so in one of them.

* For some value of "should". I don't think we ever actually use it...

Removes a couple of TODO comments to check that attributes are not already set.

@pganssle
Copy link
Member

Hm... So one of my eventual goals for the parser is having a representation of the result that retains a mapping from string to datetime to hopefully allow us to do things like cache the inference step, and to allow people to modify some of the parsing logic once they have the full mapping.

I don't really have a problem with the deduplication part of it or the idea that we should probably have a cleaner representation of the current state of the parse while we're parsing things, but if this is intended to be a public interface and it's not consistent with that goal, I'm a bit afraid that we'll introduce a lot of churn by immediately switching to a different representation of the parse state.

In any case, you have marked this as "POC", what do you want to happen with this PR? Do you want a thorough review of the interface? The implementation? Or is it just part of a gallery of possible approaches for when we finally get around to revamping the parser interface?

@jbrockmendel
Copy link
Contributor Author

So one of my eventual goals for the parser is having a representation of the result that retains a mapping from string to datetime

This is a big part of what the TokenStream class mentioned in #883 does. I'll put up a separate POC for that.

In any case, you have marked this as "POC", what do you want to happen with this PR?

This closely matches the design I landed on downstream as being roughly optimal, so I'd be happy to see this merged as-is. But since historically class-structure/interface issues have been the ones we have been least on-the-same-page about, I labelled this Proof Of Concept in the hopes that there would be a few rounds of discussion before strong opinions were formed.

we should probably have a cleaner representation of the current state of the parse while we're parsing things, but if this is intended to be a public interface and it's not consistent with that goal

Can you clarify the latter part of this, i.e. "it's not consistent with that goal"?

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