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

fix: memory leaks #64

Merged
merged 7 commits into from
May 14, 2024
Merged

fix: memory leaks #64

merged 7 commits into from
May 14, 2024

Conversation

dankinsoid
Copy link
Contributor

@dankinsoid dankinsoid commented May 1, 2024

Closes #63
Closes #65

Proposed Changes

  • Replace classes with structs and lazy vars with computed vars

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • swift test completes successfully
  • Commit messages are conventional
  • Sign CLA (if not already signed)

@bednar bednar changed the title Fix memory leaks fix: memory leaks May 2, 2024
@bednar bednar mentioned this pull request May 2, 2024
Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Thanks again for your PR 👍

Can you please satisfy our Checklist:

image

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Could you please ensure that our CI checks are satisfied as well?

image

@dankinsoid
Copy link
Contributor Author

@bednar swift test doesn't pass on master branch, it waits for http://localhost:8086/

@dankinsoid dankinsoid requested a review from bednar May 2, 2024 15:38
@dankinsoid
Copy link
Contributor Author

I don't know how to fix the last one

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Hi @dankinsoid,

Thank you very much for your Pull Request. It appears that the final check is failing due to flakiness of the tests on the macOS platform. However, we're still able to merge your PR regardless, as from my point of view, it looks perfectly fine!

If you have any other contributions or suggestions, please don't hesitate to share them. We greatly appreciate your efforts.

Best Regards.

@dankinsoid
Copy link
Contributor Author

@bednar Hello! Do I need to do smth to merge the PR? I need it for my library

@bednar
Copy link
Contributor

bednar commented May 11, 2024

Thanks again for your PR 👍. I will merge at Monday and after that we will release new version of the client.

@bednar
Copy link
Contributor

bednar commented May 13, 2024

Hi @dankinsoid,

I wanted to give you an update on the progress we're making with your Pull Request. I've prepared PR #66 to address some CI checks issues we've been facing. Once PR #66 is reviewed and merged, we will be able to proceed with merging your PR as well.

As mentioned yesterday, we are on track to release the new version of the client, which will include your fix, by the end of this week.

Thank you for your patience and your contributions. I'll keep you updated as we move forward.

Best regards

@dankinsoid
Copy link
Contributor Author

@bednar thank you!

@bednar bednar added this to the 1.7.0 milestone May 14, 2024
@bednar bednar merged commit fc8e3af into influxdata:master May 14, 2024
9 checks passed
@bednar
Copy link
Contributor

bednar commented May 14, 2024

@dankinsoid, the PR has been merged. Thank you very much once again for your contribution!

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.

PointSettings init is internal Memory leaks
2 participants