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

chore: fix a few typos #754

Merged
merged 3 commits into from
Aug 25, 2022
Merged

chore: fix a few typos #754

merged 3 commits into from
Aug 25, 2022

Conversation

Lioness100
Copy link
Contributor

This PR fixes a few typos throughout the repository. I didn't test or lint because mongodb-memory-server wasn't building for me locally and I needed to get to sleep haha.

Copy link
Member

@hasezoey hasezoey 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 finding and correcting these typos, but i have to request some changes:

  • when changing strings in tests or code, make sure the characters are properly escaped or change the string type
  • if possible, maybe split these changes out into multiple commits (like docs typo fixes and code fixes)

@hasezoey
Copy link
Member

I didn't test or lint because mongodb-memory-server wasn't building for me locally and I needed to get to sleep haha.

could you maybe post the errors and your system information? (because i am also a maintainer of that package)

  • mongodb-memory-server version (and which package is used)
  • your system (mac/linux (with distro)/windows)
  • arch (x86_64/arm64)

@hasezoey hasezoey added the enhancement Improve an existing Feature label Aug 22, 2022
@Lioness100
Copy link
Contributor Author

Hi!

  • Just to make sure I didn't miss one, the only string I forgot to escape is in src/internal/errors.ts, right?

  • Do you want 2 separate PRs? Or just two different commits for more organized viewing

  • I didn't get any errors, it was just stuck on something like "Building mongodb-memory-server" for ~7 minutes until I gave up. System: Windows 11 x64

@hasezoey
Copy link
Member

Just to make sure I didn't miss one, the only string I forgot to escape is in src/internal/errors.ts, right?

i think so

Do you want 2 separate PRs? Or just two different commits for more organized viewing

just different commits if possible, it makes it cleaner (in my opinion)

I didn't get any errors, it was just stuck on something like "Building mongodb-memory-server" for ~7 minutes until I gave up. System: Windows 11 x64

weird, if you can reproduce it, could you try again with MONGOMS_DEBUG=1 npm install --foreground-scripts (or if yarn MONGOMS_DEBUG=1 npm install) and post the full output? (or if not in a install, just add MONGOMS_DEBUG=1 infront)

Note: i dont know if this is the proper way to set environment variables on non UNIX systems


forgot to add in my last comment:

I didn't test or lint because mongodb-memory-server wasn't building for me locally and I needed to get to sleep haha.

MMS is only required for tests, it does not affect linting (though if it was the postinstall failing, you can try to reinstall everything with MONGOMS_DISABLE_POSTINSTALL=1

also, MMS is not strictly required for testing, as long as you have a mongod server already running that can be used for tests (though you need to set the config properly, which can be more work than its worth for a typo fix)

@Lioness100
Copy link
Contributor Author

Hi! Unfortunately (or fortunately) I can't reproduce the mongodb-memory-server issue (it works fine now). As for separating the changes into 2 commits, I've been trying for a while and totally entrapped myself in a web of git messups lol 😭 I'm not great at git :(

@hasezoey
Copy link
Member

I've been trying for a while and totally entrapped myself in a web of git messups lol sob I'm not great at git :(

thats fine then, it was a optional request anyway


only things needs then would be to fix the current syntax problems, then making sure the tests pass and any lint fixes if there are any

@Lioness100
Copy link
Contributor Author

Lint and tests pass locally 👍

@codecov
Copy link

codecov bot commented Aug 24, 2022

Codecov Report

Base: 97.11% // Head: 97.11% // No change to project coverage 👍

Coverage data is based on head (23203b3) compared to base (78b7eab).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #754   +/-   ##
=======================================
  Coverage   97.11%   97.11%           
=======================================
  Files          17       17           
  Lines         902      902           
  Branches      245      245           
=======================================
  Hits          876      876           
  Misses         26       26           
Impacted Files Coverage Δ
src/defaultClasses.ts 100.00% <ø> (ø)
src/hooks.ts 100.00% <ø> (ø)
src/internal/constants.ts 100.00% <ø> (ø)
src/internal/errors.ts 93.65% <ø> (ø)
src/internal/processProp.ts 95.17% <ø> (ø)
src/internal/utils.ts 97.20% <ø> (ø)
src/typegoose.ts 97.43% <ø> (ø)
src/typeguards.ts 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

just 1 minor thing left (will self-apply):

  • reverting wording for one changelog entry (just wanna keep the same meaning)

CHANGELOG.md Outdated Show resolved Hide resolved
Revert changing of wording, but still fixing the typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve an existing Feature released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants