Skip to content
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

add @mandresm contributions to the fix #1281

Merged
merged 2 commits into from
Jan 24, 2025
Merged

Conversation

mandresm
Copy link
Contributor

  • Remove keep_list syntax (if we can avoid it, I'd rather do not add more syntax here)
  • Makes sure it does the old list handling for date operations, so that date operations are still handled correctly
  • Sets the provenance accordingly

This is tested and produces a list such as discussed in #1279

@mandresm
Copy link
Contributor Author

Hey @pgierz ! This is a PR to your original branch with hopefully a few improvements. Feel free to merge if you like it :)

@mandresm mandresm requested a review from pgierz January 23, 2025 16:00
Copy link
Member

@pgierz pgierz left a comment

Choose a reason for hiding this comment

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

Looks good, but I have a question. See below:

Comment on lines +2413 to 2416
if isinstance(result, list) and date_operation:
result = result[
-1
] # should be extended in the future - here: if list (= if diff between dates) than result in seconds
Copy link
Member

Choose a reason for hiding this comment

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

Was this only the case for dates? I didn't know that...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was the case for every list, but I think the only way we had of generating lists from a string on the past was via Date operations (because date objects don't contain this functionality so that functionality ended up here, somehow)

Comment on lines +2418 to +2419
entry = ListWithProvenance(result, None)
entry.set_provenance(rhs.provenance)
Copy link
Member

Choose a reason for hiding this comment

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

I might not understand something about provenance 100% yet, but why not this version:

Suggested change
entry = ListWithProvenance(result, None)
entry.set_provenance(rhs.provenance)
entry = ListWithProvenance(result, rhs.provenance)

Copy link
Contributor Author

@mandresm mandresm Jan 24, 2025

Choose a reason for hiding this comment

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

For providing provenance values during instanciation of ListWithProvenance (or DictWithProvenance) the provenance needs to have the same structure as the original list or dict. That is to say, if you have a dict with some key-values, what you need to give in the second argument of the instancing is also a dict with the same keys, but the values are provenances. set_provenance is used to assign the same unique provenance to a whole list/or dict with provenance, which is what we want in this case (we are expanding a string into a list so we want to assign a single provenance information to the new elements of the list.

@pgierz pgierz merged commit 0032d06 into feat/eval_lists Jan 24, 2025
13 of 16 checks passed
@mandresm mandresm deleted the feat/eval_lists_miguels branch January 24, 2025 07:53
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