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

Date replace and Date get #433

Open
wants to merge 16 commits into
base: draft
Choose a base branch
from
Open

Date replace and Date get #433

wants to merge 16 commits into from

Conversation

PondiB
Copy link
Member

@PondiB PondiB commented Apr 15, 2023

Implementation for issue #254
closes #254

@PondiB PondiB requested review from soxofaan and m-mohr April 15, 2023 14:01
@m-mohr m-mohr added this to the 2.1.0 milestone Apr 15, 2023
proposals/date_get.json Outdated Show resolved Hide resolved
proposals/date_get.json Outdated Show resolved Hide resolved
proposals/date_get.json Outdated Show resolved Hide resolved
proposals/date_get.json Outdated Show resolved Hide resolved
proposals/date_replace.json Outdated Show resolved Hide resolved
proposals/date_get.json Outdated Show resolved Hide resolved
proposals/date_replace.json Outdated Show resolved Hide resolved
proposals/date_replace.json Outdated Show resolved Hide resolved
proposals/date_replace.json Outdated Show resolved Hide resolved
proposals/date_replace.json Outdated Show resolved Hide resolved
jdries added a commit to Open-EO/openeo-geotrellis-extensions that referenced this pull request Sep 25, 2023
jdries added a commit to Open-EO/openeo-geotrellis-extensions that referenced this pull request Sep 25, 2023
jdries added a commit to Open-EO/openeo-geotrellis-extensions that referenced this pull request Sep 27, 2023
m-mohr
m-mohr previously requested changes Sep 30, 2023
Copy link
Member

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

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

Not a big fan of the _component suffix, otherwise just added a couple of minor comments to clean-up the descriptions. Overall very solid proposals, I think.

Has this been tested against common date-time libraries, especially the ones that are mentioned in the implementation guide? It would be good to know whether all the edge cases have been found, documented and been tested with at least the 4 libraries mentioned there (this is the reason for the change request).

@PondiB
Copy link
Member Author

PondiB commented Oct 3, 2023

Not a big fan of the _component suffix, otherwise just added a couple of minor comments to clean-up the descriptions. Overall very solid proposals, I think.

Okay

Has this been tested against common date-time libraries, especially the ones that are mentioned in the implementation guide? It would be good to know whether all the edge cases have been found, documented and been tested with at least the 4 libraries mentioned there (this is the reason for the change request).

Not really. What implementation guide?

@m-mohr
Copy link
Member

m-mohr commented Oct 3, 2023

What implementation guide?

https://github.com/Open-EO/openeo-processes/blob/master/meta/implementation.md

@soxofaan
Copy link
Member

soxofaan commented Oct 3, 2023

Not a big fan of the _component suffix,

You're not a fan of _component as a suffix, or any suffix at all? Do you have other suggestions?

@m-mohr
Copy link
Member

m-mohr commented Oct 3, 2023

You're not a fan of _component as a suffix, or any suffix at all? Do you have other suggestions?

date_get and date_replace seem fine to me, but I'm not the dictator so if many prefer the _component suffix, I'm fine with it. I don't have a good alternative at hand (maybe "part"), I also don't think we hve anything comparable right now, which we can borrow from.

@soxofaan
Copy link
Member

soxofaan commented Oct 3, 2023

the closest comparable process is array_element I think, so date_element could make sense too

@PondiB
Copy link
Member Author

PondiB commented Nov 7, 2023

date_replace_component and date_get_component is more intuitive to me than date_replace, date_get and date_element.

@PondiB
Copy link
Member Author

PondiB commented Dec 8, 2023

@m-mohr , if there is no further review of this then kindly approve.

@PondiB
Copy link
Member Author

PondiB commented Dec 8, 2023

@soxofaan 👆🏾

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

no further remarks from me, we already iterated enough on this on I guess :)

@PondiB
Copy link
Member Author

PondiB commented Dec 8, 2023

no further remarks from me, we already iterated enough on this on I guess :)

Sure, thanks.

@PondiB PondiB closed this Dec 8, 2023
@PondiB PondiB reopened this Dec 8, 2023
@PondiB PondiB dismissed m-mohr’s stale review December 8, 2023 14:08

it's solid now.

@Open-EO Open-EO deleted a comment from PondiB Dec 8, 2023
@m-mohr
Copy link
Member

m-mohr commented Dec 8, 2023

@PondiB You did never followed-up on the comment with regards to extending the implementation guide. Any updates on that?

"minute",
"hour",
"day",
"day of week",
Copy link
Member

@m-mohr m-mohr Dec 8, 2023

Choose a reason for hiding this comment

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

Is day of week Monday or Sunday based? Does it start with 0 or 1? Does it return numbers or strings?

"day of week",
"day of year",
"week",
"week number",
Copy link
Member

@m-mohr m-mohr Dec 8, 2023

Choose a reason for hiding this comment

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

zero or one based? What's the difference between week and week number?

},
{
"name": "component",
"description": "The component for the value given. The following pre-defined components are available:\n\n- millisecond: Milliseconds\n- second: Seconds .\n- minute: Minutes\n- hour: Hours\n- day: Days - changes only the the day part of a date\n- day of week: Day of Week\n- day of year: Day of Year\n- week: Weeks (equivalent to 7 days)\n- week number: Week Number\n- month: Months\n- year: Years\n\nReplacements with the component `year`, `month` or `day` do never change the time.",
Copy link
Member

Choose a reason for hiding this comment

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

The description needs to be made much more precise with regards to value ranges for example. See comments below.

Copy link
Member

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

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

Did a quick review. See comments above. Also, the implementation guide on what libraries support exactly this should be added (compare to the other date processes).

@PondiB
Copy link
Member Author

PondiB commented Dec 12, 2023

Did a quick review. See comments above. Also, the implementation guide on what libraries support exactly this should be added (compare to the other date processes).

Cool, thanks. I'll address them later.

@Open-EO Open-EO deleted a comment from PondiB Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants