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

MLS: correct typo in principle by using principal instead #4505

Merged
merged 13 commits into from
Feb 2, 2021

Conversation

keineahnung2345
Copy link
Contributor

No description provided.

@mvieth
Copy link
Member

mvieth commented Nov 10, 2020

It is problematic to just change the API. You could add a function with the old name that calls the function with the new, correct name, and add a deprecation notice.

@keineahnung2345
Copy link
Contributor Author

Another question, the constant MLS_MINIMUM_PRINCIPAL_CURVATURE is mentioned in the comment, but I can't find it in the code, should I create this constant and replace the 1e-5 in calculatePrincipalCurvatures with it?

@mvieth
Copy link
Member

mvieth commented Nov 29, 2020

Hm, the constant was added in this commit, but later removed in the same pull request, and only the mention in the documentation remained. I kinda agree with the review comments on the addition, so I would tend to just change the documentation.

@keineahnung2345
Copy link
Contributor Author

@mvieth I've updated the doc.

@mvieth mvieth added the needs: code review Specify why not closed/merged yet label Dec 3, 2020
@kunaltyagi kunaltyagi added the changelog: deprecation Meta-information for changelog generation label Dec 5, 2020
@kunaltyagi kunaltyagi changed the title MLS: principle > principal MLS: correct typo in principle by using principal instead Dec 5, 2020
Copy link
Member

@kunaltyagi kunaltyagi left a comment

Choose a reason for hiding this comment

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

Can we move the deprecated function to the declaration site? That would make it easier to remove in future

@keineahnung2345
Copy link
Contributor Author

@kunaltyagi Sorry but what do you mean by "move to the declaration site"?

@kunaltyagi
Copy link
Member

The simple definition of the deprecated function could be moved to the header (the function declaration) so removing it post deprecation is easier.

@mvieth
Copy link
Member

mvieth commented Dec 21, 2020

Hm, now macOS fails because of the undefined inline function. Is there any other solution than to revert the last commit?

@keineahnung2345
Copy link
Contributor Author

According to mpx/lua-cjson#28, one way is removing inline...

@mvieth
Copy link
Member

mvieth commented Dec 22, 2020

You mean removing inline from calculatePrincipalCurvatures? Sure, let's give it a try.

An attempt to fix undefined inline function on MacOS
@keineahnung2345
Copy link
Contributor Author

@mvieth It works.

@mvieth mvieth requested a review from kunaltyagi February 1, 2021 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: deprecation Meta-information for changelog generation module: surface needs: code review Specify why not closed/merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants