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

Expose Matrix.User #19

Merged
merged 16 commits into from
Apr 12, 2024
Merged

Expose Matrix.User #19

merged 16 commits into from
Apr 12, 2024

Conversation

BramvdnHeuvel
Copy link
Collaborator

This pull request adds the Matrix.User module to be used by the developer.

Additionally, it adds a breaking change where the sender field of the public Event type is now a User type rather than a sender, effectively pushing for a major change.

@BramvdnHeuvel BramvdnHeuvel self-assigned this Apr 12, 2024
Copy link
Collaborator Author

@BramvdnHeuvel BramvdnHeuvel left a comment

Choose a reason for hiding this comment

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

I have looked through the code and it looks great to me. The documentation also renders nicely.

One idea for the future, however, is to be a bit more strict on combining the strings in the Text module.

@BramvdnHeuvel
Copy link
Collaborator Author

Similarly, the ParserExtra.elm might need a refactor in the future into something similar to the current Json module, where it is rebuilt to serve the purpose of the Elm SDK better. Right now, the parsers offer little explanation about what is wrong with the strings parsed.

Given how there's issues like matrix-org/matrix-spec#1506 floating around the Matrix ecosystem, it might benefit debugging if the module was more helpful on how certain strings like user IDs do not meet the requirements of the Matrix spec.

@BramvdnHeuvel BramvdnHeuvel marked this pull request as ready for review April 12, 2024 12:42
@BramvdnHeuvel BramvdnHeuvel merged commit 8c73fbf into develop Apr 12, 2024
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.

1 participant