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

Create witch custom id #225

Closed
wants to merge 14 commits into from
Closed

Create witch custom id #225

wants to merge 14 commits into from

Conversation

lsmilek1
Copy link
Contributor

@lsmilek1 lsmilek1 commented Sep 6, 2021

Added createWithCustomObjectId parameter in .save() functions to remove the need of setting isIgnoreCustomObjectIdConfig = true and allowCustomObjectId = true for a mixed custom objectId environment

@lsmilek1
Copy link
Contributor Author

lsmilek1 commented Sep 6, 2021

As this is my first time I encountered XCTests and also started updating other branch and per your comment added new branch later, I am really not sure If I got the things right. I apologise in advance!

@cbaker6
Copy link
Contributor

cbaker6 commented Sep 6, 2021

Can you resolve the conflicts?

@cbaker6
Copy link
Contributor

cbaker6 commented Sep 6, 2021

Also, it looks like you started from the branch I created as opposed to branching from the main which is why you need to resolve conflicts #220 (comment)

@lsmilek1
Copy link
Contributor Author

lsmilek1 commented Sep 6, 2021

Sure, I will have a look on the conflicts. I am sorry, I was never using git control before. I started from non-main branch, you are right (not knowing there will be such conflicts) and after you mentioned it I had actually all the code modified, so I then tried to merge it in the forked main and then create PR... Obviously that is not the way to do that.

Also, I did not know how to run the XCTests, as I never did any before and I do not have local Parse Server running (do I need that?) so if you could give me a hint that would be great!

@cbaker6
Copy link
Contributor

cbaker6 commented Sep 6, 2021

Also, I did not know how to run the XCTests, as I never did any before and I do not have local Parse Server running (do I need that?) so if you could give me a hint that would be great!

You don't need a parse server to run the tests. You can run tests locally using Xcode or they will run automatically in the CI when you fix the conflicts.

CHANGELOG.md Outdated Show resolved Hide resolved
@cbaker6 cbaker6 marked this pull request as draft September 6, 2021 15:54
CHANGELOG.md Outdated Show resolved Hide resolved
@cbaker6
Copy link
Contributor

cbaker6 commented Sep 6, 2021

Be sure to look through the failed tests and resolve them. You can press Ready for review when you fixed everything and added necessary tests.

You will probably also want to run through all of the Swift playgrounds to test your changes on a real server since you are attempting to change essential behavior.

correcting **Improvements** block wrongly assigned to already released version 1.9.8
Correcting wrong objectId reference
Correcting wrong objectId reference
Correcting failed test cases
Correcting failing test cases
@cbaker6
Copy link
Contributor

cbaker6 commented Sep 6, 2021

You need to install SwiftLint and ensure you fix all of the errors and warnings before committing or else your builds won't pass. If SwiftLint is installed, you will see the build errors and warnings on your local computer https://github.com/parse-community/Parse-Swift/blob/main/CONTRIBUTING.md

@lsmilek1
Copy link
Contributor Author

lsmilek1 commented Sep 7, 2021

I believe this is at the end way over my competence. I already see I am breaking it more than contributing, especially the tests, and in the Swift playgrounds I will have to spin up a local version of the server as I understood your comment. This is all new to me and even I somehow make the tests to pass, the test logic will be rather wrong.

The goal was to remove the need of allowCustomObjectId and isIgnoreCustomObjectIdConfig and invert the logic into one optional flag createWithCustomId to reduce the code length on each .save() as basically only few objects would make use of the allowCustomObjectId + isIgnoreCustomObjectIdConfig. I believe a wrapper .save() in the non-SDK classes will be more reasonable way to do that instead of attempting to change SDK's essential behavior.

@lsmilek1 lsmilek1 closed this Sep 7, 2021
@cbaker6
Copy link
Contributor

cbaker6 commented Sep 7, 2021

I already see I am breaking it more than contributing, especially the tests,

the tests are there to ensure core logic isn’t broken when changes are made. The changes you were making will definitely break core logic, and some of the tests that fail will need to be changed. It also allows us to see what breaks and if the change should be added.

I don’t see a commit where you fixed the linking so we can at least see the test failures to evaluate your changes. Can you fix the linting and commit your code?

and in the Swift playgrounds I will have to spin up a local version of the server as I understood your comment.

this is as simple as downloading docker, forking a repo and running a one liner (see my comment below from the readme). If back4app allows you to spin up a free instance you can use that or any other service and tear it down when you are finished. If you use something like back4app you will need to edit

ParseSwift.initialize(applicationId: "applicationId",
clientKey: "clientKey",
serverURL: URL(string: "http://localhost:1337/1")!,
allowCustomObjectId: true)

From the README:

To learn how to use or experiment with ParseSwift, you can run and edit the ParseSwift.playground. You can use the parse-server in this repo which has docker compose files (docker-compose up gives you a working server) configured to connect with the playground files, has Parse Dashboard, and can be used with mongoDB or PostgreSQL.

This is all new to me and even I somehow make the tests to pass, the test logic will be rather wrong.

I think it’s useful to think about and develop tests for added features and see how/why a change breaks current tests. It will also allow you to learn more about how the SDK is designed and then help you discover bugs in the future

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.

2 participants