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

Rename MatrixInMemoryStore to MemoryStore #861

Merged
merged 2 commits into from
Mar 20, 2019
Merged

Conversation

jryans
Copy link
Collaborator

@jryans jryans commented Mar 19, 2019

None of the other store classes use the Matrix prefix, and I find the mismatch
confusing (it leads me to think it might have a different purpose than the
others).

This change removes the prefix from the store for consistency. The old name is
left as an export for existing SDK consumers.

None of the other store classes use the `Matrix` prefix, and I find the mismatch
confusing (it leads me to think it might have a different purpose than the
others).

This change removes the prefix from the store for consistency. The old name is
left as an export for existing SDK consumers.
@jryans jryans requested a review from a team March 19, 2019 14:33
@t3chguy
Copy link
Member

t3chguy commented Mar 19, 2019

This is a breaking change as some things depend on this store. For example: https://github.com/matrix-org/matrix-search/blob/ef289c6e8b3293089a5a422c5664da208f864a52/js_fetcher/index.ts#L167

NVM, I see the double export

@turt2live
Copy link
Member

turt2live commented Mar 19, 2019

Not to mention all the bots and bridges which use it. A stub class might work, but comments which clear up the confusion might be better.

Edit: I also missed the double export. Can it be made more obvious somehow?

@jryans
Copy link
Collaborator Author

jryans commented Mar 19, 2019

The PR currently preserves exporting the old name for compatibility, as I was expecting issues just like what you've mentioned otherwise.

Does that address these issues?

@jryans
Copy link
Collaborator Author

jryans commented Mar 19, 2019

Can it be made more obvious somehow?

I could add "COMPATIBILITY:" or some other loud string to the comment, maybe?

@turt2live
Copy link
Member

It sure reminds me that the project has never been updated for new build practices, but that's a different issue.

I think it should at least get a deprecation notice so we can remove it eventually.

@jryans
Copy link
Collaborator Author

jryans commented Mar 19, 2019

Okay, I have added the deprecation notice as well. (Sorry for tripping everyone's breaking change alarm... 😰)

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

Looks like all the instances of it. I assume this has been tested, and that it doesn't cause stuff to explode.

Thanks for deprecating it, and not making it a breaking change from the outset despite everyone's efforts to call you out on it :)

@jryans
Copy link
Collaborator Author

jryans commented Mar 20, 2019

Yes, I've tested that both the old and new exported names are working as expected.

@jryans jryans merged commit 6a57ddd into develop Mar 20, 2019
@t3chguy t3chguy deleted the jryans/storage-edge-cases branch May 10, 2022 14:21
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