Skip to content

Conversation

@idiotsky
Copy link
Contributor

No description provided.

@axunonb
Copy link
Collaborator

axunonb commented Dec 11, 2024

Thanks for submitting the PR.
Actually there several things to consider while heading to a new v5.
Please understand the comments under this aspect.

The Organizer class might require more similar refactoring as proposed in the PR.
Examples:

  • The class should up updated to use NRT.
  • Should the setter for SentBy remove an existing Parameters entry?
  • The getter for DirectoryEntry causes a similar issue. it should return null if no DIR entry exists.
  • Decision about errors when creating the Uri - throw or use null instead? This may also concern the ParameterCollectionProxy.Set method.
  • Unit tests missing

@minichma What do you think. Should we fix this issue in an isolated or a "fix it all" context?

@minichma
Copy link
Collaborator

I agree. This PR certainly is beneficial, but the same improvement as for SentBy should also be applied to DirectoryEntry at least.

@idiotsky Would you be ready to adjust the PR accordingly?

Generally speaking, I think that representing these values as Uri is not the best thing to do, so at some point we might consider replacing all the Uri values by simple string ones.

@idiotsky
Copy link
Contributor Author

I agree. This PR certainly is beneficial, but the same improvement as for SentBy should also be applied to DirectoryEntry at least.

@idiotsky Would you be ready to adjust the PR accordingly?

Generally speaking, I think that representing these values as Uri is not the best thing to do, so at some point we might consider replacing all the Uri values by simple string ones.

I agree with replace the Uri to string is better idea.

Nullable reference types were enabled in the `Organizer` class, ensuring properties like `SentBy`, `DirectoryEntry`, and `Value` handle nullability appropriately. A new `Journal3` ICS file and its associated test were added to validate `SENT-BY` behavior in journals.
@sonarqubecloud
Copy link

@idiotsky
Copy link
Contributor Author

I submit the changes, any feed back, I'll try to fix it.

Copy link
Collaborator

@minichma minichma left a comment

Choose a reason for hiding this comment

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

LGTM

@axunonb axunonb merged commit 3c5a596 into ical-org:main Dec 15, 2024
4 checks passed
@minichma
Copy link
Collaborator

@idiotsky Thank you!

@idiotsky
Copy link
Contributor Author

@idiotsky Thank you!

you are welcome

@Sansoeko
Copy link

Sansoeko commented Jan 3, 2025

Closes #587

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.

4 participants