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

Add toleration for StringRecordId (#372) #373

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Conversation

knackstedt
Copy link
Contributor

Thank you for submitting this pull request! We appreciate you spending the time to work on these changes.

What is the motivation?

PR #372.

What does this change do?

Checks if the provided argument is already an instance of a StringRecordId.

What is your testing strategy?

There is not already a test file for StringRecordId. My code change adds 2 lines and it's simple enough to know it's not going to implode.

Is this related to any issues?

#372

Have you read the Contributing Guidelines?

src/data/types/recordid.ts Outdated Show resolved Hide resolved
@kearfy
Copy link
Member

kearfy commented Nov 11, 2024

Hey @knackstedt, thank you for this PR! Instead of casting the value to any and back to StringRecordId, could you make the argument of the constructor accept string | StringRecordId?

@knackstedt
Copy link
Contributor Author

@kearfy Thanks, I've made that alteration via a force change to keep history clean. Does that look good?

src/data/types/recordid.ts Outdated Show resolved Hide resolved
@kearfy
Copy link
Member

kearfy commented Nov 12, 2024

Awesome thanks @knackstedt! There was just one more formatting issue but I resolved that for you. I'll try to look at your other issues soon!

@kearfy kearfy merged commit 6ecf455 into surrealdb:main Nov 12, 2024
10 checks passed
@knackstedt
Copy link
Contributor Author

Awesome, thanks @kearfy! I didn't make PRs for the other issues -- They seemed like they needed your concensus and understanding of the internals. I did mention them to Ignacio though, FWIW

@kearfy
Copy link
Member

kearfy commented Nov 12, 2024

Yeah, Ignacio let me know as well 👍
I've got some other priorities to handle first, but will get to them soon

@knackstedt knackstedt mentioned this pull request Nov 13, 2024
1 task
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