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

Replace f0 type suffix with f, rename Rect types #97

Merged
merged 2 commits into from
Jul 21, 2021

Conversation

knuesel
Copy link
Contributor

@knuesel knuesel commented Oct 3, 2020

This implements the change proposed in #61 . If the GeometryBasics maintainers are OK with this, I'll make PRs for all the dependents.

Update:

The renamed types are:

Vecf0    → Vecf
Vec1f0   → Vec1f
Vec2f0   → Vec2f
Vec3f0   → Vec3f
Vec4f0   → Vec4f
Pointf0  → Pointf
Point1f0 → Point1f
Point2f0 → Point2f
Point3f0 → Point3f
Point4f0 → Point4f
Mat1f0   → Mat1f
Mat2f0   → Mat2f
Mat3f0   → Mat3f
Mat4f0   → Mat4f
Rect2D   → Rect2 
Rect3D   → Rect3 
FRect    → Rectf
FRect2D  → Rect2f 
FRect3D  → Rect3f 
IRect    → Recti
IRect2D  → Rect2i 
IRect3D  → Rect3i 
TRect    → RectT

The following script can be used to refactor files automatically (it should work on Linux or other systems with GNU tools):

#!/bin/bash

set -eu

if (($# == 0)); then 
    echo "Usage: ${0##*/} <file or directory>..." >&2
    exit 1
fi

declare -A FromTo
FromTo=(
  ['\<(Point|Mat|Vec|RGB|RGBA|Quaternion)([0-9]?)f0\>']='\1\2f'
  ['\<(([FI])|(T))?Rect(([23])D)?\>']='Rect\5\L\2\E\3'
)

for from in "${!FromTo[@]}"; do
    to=${FromTo[$from]}
    grep -EIrl -- "$from" "$@" | xargs --no-run-if-empty sed -E "s/$from/$to/g" -i
done

Typical usage is to run ./refactor-f0-Rect.sh * to refactor the files (recursively) in a repository, then inspect the result with git diff (it's fairly reliable but might do unwanted replacements e.g. in binary files).

@juliohm
Copy link
Member

juliohm commented Oct 6, 2020

We are trying to clean up the whole Vecf, Pointf story. The refactoring of the code is taking place in the cleanup branch.

@SimonDanisch
Copy link
Member

I'll make PRs for all the dependents.

That'd be amazing :-0

@SimonDanisch
Copy link
Member

We should also properly deprecate the old names!

@juliohm juliohm mentioned this pull request Oct 7, 2020
@knuesel
Copy link
Contributor Author

knuesel commented Oct 7, 2020

I hadn't followed the developments on the cleanup branch, which look like a more ambitious refactoring. I'll post some ideas in #100.

@SimonDanisch indeed, proper deprecation was not (yet) in my PR. It's not clear though if it makes sense to keep this one, with the other changes going on in cleanup.

@SimonDanisch
Copy link
Member

It's not clear though if it makes sense to keep this one, with the other changes going on in cleanup.

Cleanup is a much larger project, this one could be merged fairly quickly if you can open those PRs & we deprecate the old binding correctly ;)

@knuesel
Copy link
Contributor Author

knuesel commented Jul 16, 2021

Hi, I have some time in the coming days to finish this 🙂 I've rebased the PR on master and added deprecation warnings for the old types.

There's a corresponding Makie PR at MakieOrg/Makie.jl#1132.

While we are at it, I propose to add the following renames to this PR:

Rect2D, Rect3D -> Rect2, Rect3
FRect, FRect2D, FRect3D -> Rectf, Rect2f, Rect3f
IRect, IRect2D, IRect3D -> Recti, Rect2i, Rect3i
TRect -> RectT

This would be nice for internal consistency and is also easier to type, and consistent with what some other libraries do (see e.g. https://docs.opencv.org/3.4/dc/d84/group__core__basic.html).

What do you think?

@knuesel knuesel changed the title Replace f0 type suffix with f Replace f0 type suffix with f, rename Rect types Jul 19, 2021
@knuesel
Copy link
Contributor Author

knuesel commented Jul 19, 2021

@SimonDanisch I have added the Rect renames with deprecation warnings. For me this PR is ready.

@SimonDanisch
Copy link
Member

Awesome thank you!

@knuesel
Copy link
Contributor Author

knuesel commented Jul 20, 2021

You're welcome! I've made the PRs to the closely related projects I could find. For other (third-party) projects I think it makes more sense to wait for new releases of GeometryBasics and Makie, so I can make PRs that work with the new versions.

Let me know if there's anything else to do to get this merged!

@SimonDanisch
Copy link
Member

Thank you for all the effort! Lets merge this!

@knuesel
Copy link
Contributor Author

knuesel commented Jul 20, 2021

@SimonDanisch was the 0.4.0 release supposed to include this?

@SimonDanisch
Copy link
Member

Lol, what happened here :-O Yeah, definitely 😅

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.

None yet

3 participants