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.

Sign up for GitHub

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

Jump to bottom

Resolve various typos throughout the SDK #63

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Resolve various typos throughout the SDK #63

wants to merge 8 commits into from
+134 −127

Conversation

jstart
Copy link

@jstart jstart commented Sep 17, 2024

Description

This PR resolves various typos found throughout the project in code comments and class/struct/function names. Not sure how helpful this is and I don't want to pollute the git history if that is a concern. Just wanted to clean up a few things as I get familiar with the codebase! :)

Checklist

@jstart jstart requested a review from a team as a code owner September 17, 2024 06:58
ArielDemarco
ArielDemarco reviewed Sep 17, 2024
View reviewed changes
Sources/EmbraceCore/FileSystem/EmbraceFileSystem.swift Outdated
Comment on lines 56 to 57
/// - partitionIdentifier: The main paritition identifier to use
/// - partitionIdentifier: The main partitionfetched remote config
/// identifier to use
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could it be that there was some kind of error here when you were typing?

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes thanks for catching that, did an incorrect find and replace!

@ArielDemarco
Copy link
Collaborator

Hey @jstart! Thank you very much for the PR! You did a huge code review! And it's very helpful because anything that improves code readability is always beneficial 😁
I made a couple of brief comments, but overall it looks very good!

@ArielDemarco
Copy link
Collaborator

@jstart seems there's a compiling issue on SessionRecordTests regarding a typo you fixed: fetchOldestSesssion to fetchOldestSession. Seems like it's missing a replacement there too:
-> ❌ /Users/runner/work/embrace-apple-sdk/embrace-apple-sdk/Tests/EmbraceStorageInternalTests/SessionRecordTests.swift:275:35: value of type 'EmbraceStorage' has no member 'fetchOldestSesssion'

@jstart
Copy link
Author

jstart commented Sep 17, 2024

SessionRecordTests

Ah thanks for catching that! I'll make sure all the test targets build and run.

@jstart
Copy link
Author

jstart commented Oct 3, 2024

@ArielDemarco I think I got the tests green finally! Let me know if you think any other changes are required!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ArielDemarco ArielDemarco ArielDemarco left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jstart @ArielDemarco