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

Refactor JavaScript integration and improve code readability. #164

Merged
merged 1 commit into from
Oct 1, 2023

Conversation

nmerget
Copy link
Collaborator

@nmerget nmerget commented Sep 26, 2023

Hey, I'm not into C++ so I did some try/error to fix the issues for the CI.

  • Update .github/actions:

    • Moved the version to another workflow_call to reuse it for better maintenance (I going to open up an issue with my suggestions)
    • fixed issue with linux builds by removing werror=yes, which could be enabled again but Wno-error="cast-function-type" didn't work with SCONSFLAGS
    • removed dev_builds for linux because of the debugger
  • removing/changing issues inside the ci:

    • some issues from this PR
    • disabling the debugger for now to make sure the pipeline won't fail

This branch should be squashed 👍

@nmerget nmerget marked this pull request as draft September 26, 2023 22:57
@fire
Copy link
Collaborator

fire commented Sep 27, 2023

I approved the github actions. Been busy so if someone can continue the work on #156 that'll be amazing.

@fire
Copy link
Collaborator

fire commented Sep 30, 2023

I think the js debugger is still being used so the build complains about needing it.

@nmerget nmerget marked this pull request as ready for review October 1, 2023 10:40
@nmerget
Copy link
Collaborator Author

nmerget commented Oct 1, 2023

@fire the pipeline is running now

The first run takes very long especially for windows, I don't know if this is a normal behavior when building the editor. After the cache is initialized it is way faster.

Please squash this branch before merging, I added a lot of testing commits because of the pipeline, we don't need them in the history

I propose merging the open PRs to have a working 4.1 branch.
Afterward, I would like to structure the repo differently and add some extra workflow_calls to publish the artifacts: #165

@fire
Copy link
Collaborator

fire commented Oct 1, 2023

Can you squash? I want to keep attribution intact

@fire fire force-pushed the fix-gd4-rebase-2 branch 2 times, most recently from 6184ea1 to 70b9c21 Compare October 1, 2023 14:04
We have refactored the JavaScript integration to make it more efficient. We have also improved the readability of the code by adding comments and restructuring the code blocks. This will make it easier for other developers to understand and contribute to the project.

Co-Authored-By: NicolasMerget <[email protected]>
@fire fire changed the title fix: gd4 rebase Refactor JavaScript integration and improve code readability. Oct 1, 2023
@fire fire merged commit 7fecf15 into godotjs:gd4-rebase Oct 1, 2023
@fire
Copy link
Collaborator

fire commented Oct 1, 2023

Thanks

@nmerget nmerget deleted the fix-gd4-rebase-2 branch October 4, 2023 17:53
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