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

null-safety & some other cleanup #16

Conversation

k9withabone
Copy link
Contributor

@k9withabone k9withabone commented Sep 3, 2022

Cleans up the null-safety migration and does a little other cleanup.

repeatAfter: repeatAfter ?? this.repeatAfter,
subtasks: subtasks ?? this.subtasks,
labels: labels ?? this.labels,
attachments: attachments ?? this.attachments,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea of the copyWith method is that it returns a copy of the Task with the modifications specified.

@k9withabone
Copy link
Contributor Author

I think the check might be failing because there is a new version of Flutter available and it's trying to use the newer versions which are incompatible with the versions in the pubspec.lock maybe?

@k9withabone k9withabone marked this pull request as ready for review September 3, 2022 16:34
Copy link
Collaborator

@Benimautner Benimautner left a comment

Choose a reason for hiding this comment

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

In general I'm okay with all the changes. Also, I'm sorry, some changes I made in the migrations were not well though through, but my schedule has been very full in the last couple of weeks.

Copy link
Collaborator

@Benimautner Benimautner left a comment

Choose a reason for hiding this comment

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

If we iron out those last few things I think this is ready to merge!

Comment on lines 46 to 47
'created': created.toIso8601String(),
'updated': updated.toIso8601String(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

.toUtc is needed here and in every other toJSON as the API fails to handle timestamps with timezones correctly. I'm not sure if this is the API's fault or flutter's. toUtc is a workaround, we should probably address this at some point in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, seems to work for me. When I made requests directly (in python) it was giving me the proper timezone and it didn't seem to have difficulty when I gave it back. I added the .toUtc() anyway.
Regardless, I'm not sure these are editable for the client as the documentation for 'updated' says:

updated
string
A timestamp when this task was last updated. You cannot change this value.

So perhaps we should remove the 'created' and 'updated' from the toJSONs?

@Benimautner Benimautner merged commit f9c9cdc into go-vikunja:null-safety-migration Sep 7, 2022
@Benimautner
Copy link
Collaborator

Thanks so much for your work! Merged it and will release a new version soon

@kolaente
Copy link
Member

kolaente commented Sep 7, 2022

Nice work you two!

@k9withabone
Copy link
Contributor Author

Don't merge it into main quite yet. Just realized there is a couple other quirks, such as not being able to remove due dates. I'll submit another PR shortly.

@Benimautner
Copy link
Collaborator

Sorry, read your comment too late. Create a new PR and I'll merge it.

@k9withabone k9withabone mentioned this pull request Sep 7, 2022
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.

3 participants