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

Correct grammar in snapshot missing message #1142

Merged
merged 2 commits into from
Sep 8, 2022
Merged

Conversation

vadi2
Copy link
Contributor

@vadi2 vadi2 commented Sep 4, 2022

No description provided.

Copy link
Contributor Author

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

It's noticeable if you copy/paste the error message into the Chrome browser - their new spellcheck detection picks it up.

@jafeltra jafeltra self-assigned this Sep 6, 2022
@jafeltra
Copy link
Collaborator

jafeltra commented Sep 6, 2022

@vadi2 thanks for the PR! This is a nice, simple fix.

I ran the automated checks, and it looks like this is failing the check that runs the unit tests. It looks like this test in test/fhirtypes/StructureDefinition.test.ts is failing because the new wording is not reflected in the expected text. You can run the tests by running npm test, which should show this test failing. You can also run npm run check in order to run the equivalent checks that will be run on the PR, which includes the unit tests, a linting check, and a check for the formatting defined by prettier.

Once that test is fixed, this PR looks good to me. Our team requires two reviews on PRs, so someone else from our team will review it as well.

@vadi2
Copy link
Contributor Author

vadi2 commented Sep 8, 2022

@jafeltra sure, done!

I edited the file online using github.dev for convenience, I hope the tests are fine now though.

Copy link
Collaborator

@jafeltra jafeltra left a comment

Choose a reason for hiding this comment

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

Thanks for updating the test! The checks pass, so this looks good to me.

Once another reviewer reviews and approves, we'll merge this in and it will get included in the next release.

@cmoesel cmoesel self-assigned this Sep 8, 2022
Copy link
Member

@cmoesel cmoesel left a comment

Choose a reason for hiding this comment

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

Thanks @vadi2!

@cmoesel cmoesel merged commit 7423e48 into FHIR:master Sep 8, 2022
@vadi2 vadi2 deleted the patch-1 branch September 8, 2022 14:48
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