Skip to content

fix(date): persist all attributes passed by options #735

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

Merged
merged 2 commits into from
May 28, 2024

Conversation

seflue
Copy link
Contributor

@seflue seflue commented May 25, 2024

Store all attributes passed as option via Date:set, especially date_only.

I am raising this as a separate PR and precondition to PR #650. If we don't treat every field in the given date consequently, we get suprising behavior if some of the fields are present within the given "time" parameter. The idea of the function is, to allow adjusting an existing date instance by a subset of it's fields given as a table and returns the adjusted date as new value from the method.

This fix ensures, that every field can actually be adjusted within this function. I observed bugs while working on my time picker, if I used this function to get a Date object with adjusted time values (hours, minutes).

@kristijanhusak
Copy link
Member

Store all attributes passed as option via Date:set, especially date_only.

Do you need this only for date_only ? I'd prefer allowing overwriting as less as possible options to avoid side effects.
If yes, let's start only with date_only and we can extend it if necessary.
I want to revisit the whole Date class since it became a bit confusing, so I want to make sure to minimize the unnecessary changes.

@seflue
Copy link
Contributor Author

seflue commented May 27, 2024

Store all attributes passed as option via Date:set, especially date_only.

Do you need this only for date_only ? I'd prefer allowing overwriting as less as possible options to avoid side effects. If yes, let's start only with date_only and we can extend it if necessary.

I think it is mainly date-only.

I want to revisit the whole Date class since it became a bit confusing, so I want to make sure to minimize the unnecessary changes.

Initially I thought, why not make everything consistently changable, because this method from_time_table actually allows to create precise copies of existing date objects and to adjust only the fields someone wants to change, which is in my eyes cleaner, than creating the copy and having to assign the values manually. Leaving some fields out of this method leads to more surprising behavior from a callers perspective and is actually more a side-effect than treating every field equally (which is plain and simple in my eyes).
The current behavior is confusing, but it doesn't prevent the programmer to actually achieve, what they want, it's just more cumbersome and needs inside knowledge of the method.

If you still insist to treat some fields differently than others, I can try to minimize the changes, but then we should at least document, which fields can be adjusted by this method and which not, and, most importantly, why they are not adjustable. Could you provide me some field-specific why-arguments, then I would include this into the documentation.

@kristijanhusak
Copy link
Member

The current behavior is confusing, but it doesn't prevent the programmer to actually achieve, what they want, it's just more cumbersome and needs inside knowledge of the method.

Lua unfortunately does not allow (easily) to have private methods and fields. I need to revisit annotations to set what should be considered private and what is public.

If you still insist to treat some fields differently than others, I can try to minimize the changes, but then we should at least document, which fields can be adjusted by this method and which not, and, most importantly, why they are not adjustable. Could you provide me some field-specific why-arguments, then I would include this into the documentation.

I'd like to leave the things that are expected to be changeable, like what we currently have (day, month, year, hour, min, sec), and we can allow changing active and date_only.
Other things are mostly internal, and those should not be manipulated directly. Some of them are recalculated anyway in the new() method, like timestamp, dayname, but others are not. The more we directly expose, the more we need to maintain later. I don't believe a lot of people are using the Date class outside of the ordinary, but I still want to be on the safe side.

I agree that it needs to be documented. I would do that with a proper annotation on the set() method, since currently it says only table.
For now, lets just add what is necessary for your PR that adds time to the calendar, and once I revisit the whole Date class I'll add proper types and limitations where necessary.

seflue and others added 2 commits May 27, 2024 23:52
Store all attributes passed as option via Date:set, especially date_only.
Minimize adjustable attributes to the smallest possible subset, that is
actually needed.
@seflue
Copy link
Contributor Author

seflue commented May 27, 2024

I minimized the set of changeable fields to the minimum necessary for my PR.

@kristijanhusak kristijanhusak merged commit 46c839b into nvim-orgmode:master May 28, 2024
6 checks passed
@seflue seflue deleted the fix_date branch May 28, 2024 20:57
SlayerOfTheBad pushed a commit to SlayerOfTheBad/orgmode that referenced this pull request Aug 16, 2024
* fix(date): persist all attributes passed by options

Store all attributes passed as option via Date:set, especially date_only.

* chore: minimize adjustable attributes

Minimize adjustable attributes to the smallest possible subset, that is
actually needed.

---------

Co-authored-by: Sebastian Flügge <[email protected]>
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