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

Throw if connect() is called before claiming access #1269

Merged
merged 3 commits into from
May 27, 2022

Conversation

ssisk
Copy link
Contributor

@ssisk ssisk commented May 27, 2022

Fixes #1198

Hello! I ran into this issue and I thought I'd go ahead and submit a fix since it seems like a nice small change.

Copy link
Member

@raucao raucao left a comment

Choose a reason for hiding this comment

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

Welcome @ssisk! And thanks for your first contribution! 🎉

I just have a minor nitpick about wording, but overall it's looking good to me.

test.assert(e.message.includes("scope"), true, "Did not throw an error that had a message describing the problem");
sawError = true;
}
test.assert(sawError, true, "Did not throw an error when scope was not set");
Copy link
Member

@raucao raucao May 27, 2022

Choose a reason for hiding this comment

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

The descriptions of the assertions should be human-readable language of what is being asserted. When the test fails, it will then show that e.g. "throws an error when scope is not set" was expected, but the source code did not satisfy the expectation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I've updated the assertions' text so that they should match the convention, let me know if not.

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks!

Copy link
Member

@raucao raucao left a comment

Choose a reason for hiding this comment

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

👍

@raucao raucao merged commit cf9c012 into remotestorage:master May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Check if access is claimed when connecting
3 participants