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

Feature/func renaming #378

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Conversation

you-win
Copy link
Contributor

@you-win you-win commented Aug 10, 2023

  • Modify func macro to accept a 'rename = ...' attribute
  • Add integration tests to cover renamed functions

Comments

Continuing from the previous PR as GitHub (sensibly) closes the PR if you change the branch name, and I wasn't able to re-open the PR.

Please let me know if it would be better to break this functionality out like with field_var.rs. I tried to keep the number of changed files to a minimum.

There are also TODOs left in the file as a starting point for discussion, but feel free to start picking at the PR wherever you want. Most have been resolved. There is a TODO left in the code to indicate where default values on the GDScript side might be implemented.

Still todo

  • signal renaming
  • docs
  • unit tests? Covered by integration tests
  • type alias Function to Signal for readability?
  • location of FuncDefinition in src/method_registration/mod.rs is okay?
  • refactor of src/godot_api.rs:extract_attributes is okay?

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-378

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks for the update!

Continuing from the previous PR as GitHub (sensibly) closes the PR if you change the branch name, and I wasn't able to re-open the PR.

I don't think many people care about the branch name 😉 the PR title is more important. In the future, please try to keep it in one pull request, because now all the review and change history is gone, which makes things a bit harder to follow.

godot-macros/src/godot_api.rs Outdated Show resolved Hide resolved
godot-macros/src/godot_api.rs Outdated Show resolved Hide resolved
godot-macros/src/godot_api.rs Outdated Show resolved Hide resolved
godot-macros/src/godot_api.rs Outdated Show resolved Hide resolved
godot-macros/src/godot_api.rs Outdated Show resolved Hide resolved
@you-win you-win force-pushed the feature/func-renaming branch from a7baf0d to 5f70989 Compare August 10, 2023 07:30
@you-win
Copy link
Contributor Author

you-win commented Aug 10, 2023

Will squash the commits once godot_api::extract_attributes discussion is resolved.

@you-win you-win force-pushed the feature/func-renaming branch 2 times, most recently from 79df477 to 0a068d7 Compare August 10, 2023 07:51
@you-win you-win marked this pull request as ready for review August 10, 2023 07:54
@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Aug 10, 2023
@you-win you-win force-pushed the feature/func-renaming branch from 0a068d7 to d74aa47 Compare August 10, 2023 16:34
* Modify func macro to accept a 'rename = ...' attribute
* Add integration tests to cover renamed functions
@you-win you-win force-pushed the feature/func-renaming branch from d74aa47 to d6589fc Compare August 10, 2023 16:35
@you-win
Copy link
Contributor Author

you-win commented Aug 10, 2023

Ready for re-review.

Changes from the initial reviews have been made sans the extract_attributes refactor.

  • remove type alias for Signal attributes
  • split out GodotFn into a reusuable struct as FuncDefinition
  • handle parser.handle_expr errors properly and also appease clippy lint

@Bromeon
Copy link
Member

Bromeon commented Aug 11, 2023

Thanks a lot for the feature and all the amendments 🙂

@Bromeon Bromeon added this pull request to the merge queue Aug 11, 2023
Merged via the queue into godot-rust:master with commit b4e6fd6 Aug 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants