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

Catty-237: Convert UserVariable from Objective-C to Swift #1328

Merged
merged 1 commit into from May 6, 2020

Conversation

neel-makhecha
Copy link
Contributor

Converted UserVariable class from Objective-C to Swift with various other changes.

Your checklist for this pull request

Please review the contributing guidelines and wiki pages of this repository.

  • Include the name of the Jira ticket in the PR’s title
  • Verify that the Jira ticket is in the status Ready for Development
  • Include a summary of the changes plus the relevant context
  • Choose the proper base branch (develop)
  • Confirm that the changes follow the project’s coding guidelines
  • Verify that the changes generate no compiler or linter warnings
  • Perform a self-review of the changes
  • Verify to commit no other files than the intentionally changed ones
  • Include reasonable and readable tests verifying the added or changed behavior
  • Confirm that new and existing unit tests pass locally
  • Check that the commits’ message style matches the project’s guideline
  • Stick to the project’s git workflow (rebase and squash your commits)
  • Verify that your changes do not have any conflicts with the base branch
  • After the PR, verify that all CI checks have passed
  • Post a message in the #catty Slack channel and ask for a code reviewer

@m-herold
Copy link
Member

Please fix tests.

@m-herold
Copy link
Member

Do not merge this PR yet. As mentioned in the Jira ticket, this PR is blocked by CATTY-236.

@m-herold
Copy link
Member

Any news on that pull request?

@neel-makhecha
Copy link
Contributor Author

Any news on that pull request?

I have been working on this. There was a first runtime issue where null pointer from objective-C function call was being passed to a non-optional argument of isEqual method, which is fixed by making it optional.
But there is still another runtime exception that I am receiving in XMLAbstractTest and trying to track that down. The 'project' instance is getting nil, which is force unwrapped. I am attaching a screenshot for the same too... it would be very helpful if you can point some right directions to look for.
Thank you
Screenshot 2020-04-30 at 12 23 17 AM

@m-herold
Copy link
Member

If you push your most recent changes for the isEqual method I will have a look at it.

@neel-makhecha
Copy link
Contributor Author

If you push your most recent changes for the isEqual method I will have a look at it.

Done!

@m-herold
Copy link
Member

m-herold commented Apr 30, 2020

Your issue occurs because of some Objective C reflection we use in the deprecated Parser for old Catrobat language versions. Simply adding @objc(UserVariable) to your newly created Swift class should resolve the problem.

@neel-makhecha
Copy link
Contributor Author

Your issue occurs because of some Objective C reflection we use in the deprecated Parser for old Catrobat language versions. Simply adding @objc(UserVariable) to your newly created Swift class should resolve the problem.

Thanks! It's now fixed. Please review.

Copy link
Member

@m-herold m-herold left a comment

Choose a reason for hiding this comment

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

This pull request is not compiling. Please revise.

@neel-makhecha neel-makhecha force-pushed the CATTY-237 branch 2 times, most recently from 1cf4f2c to b831255 Compare May 1, 2020 17:13
@neel-makhecha
Copy link
Contributor Author

neel-makhecha commented May 1, 2020

This pull request is not compiling. Please revise.

Hi Michael, everything seems okay now, I just had to rebase and update a few method calls as per a slightly new signature of isEqual. Please review.

src/Catty/DataModel/UserVariable/UserVariable.swift Outdated Show resolved Hide resolved
src/Catty/DataModel/UserVariable/UserVariable.swift Outdated Show resolved Hide resolved
src/Catty/DataModel/Formula/FormulaElement.m Outdated Show resolved Hide resolved
src/Catty/DataModel/UserVariable/UserVariable.swift Outdated Show resolved Hide resolved
src/Catty/DataModel/UserVariable/UserVariable.swift Outdated Show resolved Hide resolved
src/Catty/DataModel/UserVariable/UserVariable.swift Outdated Show resolved Hide resolved
@neel-makhecha
Copy link
Contributor Author

Hi Michael, Please review the latest commit.

Copy link
Member

@m-herold m-herold left a comment

Choose a reason for hiding this comment

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

Thanks! Really great to see that the code base's quality is gradually increasing. I have attached a few suggestions.

src/Catty/DataModel/UserVariable/UserVariable.swift Outdated Show resolved Hide resolved
src/CattyTests/Project/ProjectTests.swift Show resolved Hide resolved
src/CattyTests/Project/ProjectTests.swift Show resolved Hide resolved
src/CattyTests/DataModel/UserVariableTests.swift Outdated Show resolved Hide resolved
src/CattyTests/Bricks/SetLookBrickTests.swift Show resolved Hide resolved
src/Catty/DataModel/UserVariable/UserVariable.swift Outdated Show resolved Hide resolved
src/Catty/DataModel/UserVariable/UserVariable.swift Outdated Show resolved Hide resolved
src/Catty/DataModel/UserVariable/UserVariable.swift Outdated Show resolved Hide resolved
src/Catty/DataModel/UserVariable/UserVariable.swift Outdated Show resolved Hide resolved
@neel-makhecha
Copy link
Contributor Author

Hi Michael, please review the latest commits.

Copy link
Member

@m-herold m-herold left a comment

Choose a reason for hiding this comment

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

Thank you! Please make those few final changes and squash your commits.

src/Catty/PlayerEngine/Scene/CBScene.swift Outdated Show resolved Hide resolved
src/CattyTests/Bricks/ShowTextBrickTests.swift Outdated Show resolved Hide resolved
src/CattyTests/SceneTests.swift Outdated Show resolved Hide resolved
@neel-makhecha
Copy link
Contributor Author

Done!

@m-herold m-herold merged commit 7907d23 into Catrobat:develop May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants