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.

Sign up for GitHub

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

Jump to bottom

Setup Hello World exercise #3

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Setup Hello World exercise #3

merged 1 commit into from
Dec 12, 2023

Conversation

pfertyk
Copy link
Contributor

@pfertyk pfertyk commented Feb 26, 2023

This is still work in progress, please don't merge. I'll add the missing pieces one-by-one, then ask for a review.

@iHiD
Copy link
Member

iHiD commented Feb 26, 2023 •
edited
Loading

Thanks 🙂

Two things:

  1. You can set PRs to be drafts until they're ready (see screenshot below)
  2. Please get a 👍 off @ErikSchierboom before merging this first PR just to check everything looks good from an admin perspective! 🙂

Screenshot 2023-02-26 at 22 38 58

@pfertyk pfertyk marked this pull request as draft February 27, 2023 22:17
@BNAndras BNAndras mentioned this pull request Sep 21, 2023
Merged
@pfertyk pfertyk force-pushed the setup-hello-world branch 2 times, most recently from 94ff2c8 to 3418bbe Compare November 27, 2023 18:32
@pfertyk pfertyk force-pushed the setup-hello-world branch 23 times, most recently from f4ffc3b to 428c323 Compare December 4, 2023 22:18
@pfertyk pfertyk marked this pull request as ready for review December 4, 2023 23:17
BNAndras
BNAndras reviewed Dec 4, 2023
View reviewed changes
.github/workflows/test.yml Outdated
runs-on: <image-name>
runs-on: ubuntu-22.04
container:
image: exercism/gdscript-test-runner

steps:
- name: Checkout repository
uses: actions/checkout@v3
Copy link
Member

Choose a reason for hiding this comment

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

General practice for Exercism now is to pin actions to a particular commit SHA.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to the SHA used by Crystal and Python tracks, thanks :)

exercises/practice/hello-world/hello_world_test.gd
@@ -0,0 +1,2 @@
func test_hello_world(solution_script):
return ["Hello, World!", solution_script.hello()]
Copy link
Member

Choose a reason for hiding this comment

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

Minor nitpick but the indent seems larger here than in hello_world.gd so one of them probably needs to be updated to match the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I've missed this 👍 Since Godot editor uses tabs by default, I think we should use them too (consistently, in all files). I've already changed the files :)

Side note: I think that Godot will accept both types of indentation, as long as they are consistent within a file, but there is no need to confuse GDScript learners :P

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I think you just need to ping Erik to take a final look.

Copy link
Member

Choose a reason for hiding this comment

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

It might be good to choose a formatter / linter if available to make sure everything is consistent between contributors and PRs. https://github.com/Scony/godot-gdscript-toolkit seems promising since it has both. There's a GitHub Action for static analysis, but we could also use the CI for checking if a PR is well-formatted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a new workflow with that toolkit. It does detect some errors (e.g. an unnecessary return) but it doesn't detect things like mixed indent (tabs vs spaces). Nevertheless, I think it's a good idea to have the linter already configured, as it might get better in the future, and it already helps a bit. Thanks for this suggestion! ;)

@pfertyk pfertyk force-pushed the setup-hello-world branch 11 times, most recently from af91056 to d0bd7d2 Compare December 11, 2023 20:14
@pfertyk
Copy link
Contributor Author

pfertyk commented Dec 11, 2023

@ErikSchierboom do you think this PR is ready, or should I add something?

@pfertyk pfertyk force-pushed the setup-hello-world branch 5 times, most recently from 005898c to 92a0683 Compare December 11, 2023 21:22
@BNAndras
Copy link
Member

gradual isn't a supported tag so the configlet CI will fail until that's either removed or configlet supports the tag being present. We shouldn't merge a PR until all the CI checks pass.

@pfertyk
Copy link
Contributor Author

pfertyk commented Dec 11, 2023

I agree :) There is a discussion in another PR about either adding gradual to the list of supported types, or having both static and dynamic types added to GDScript's track. If necessary, I will update the tags here after a decision is made ;)

@ErikSchierboom
Copy link
Member

I've opened a PR to add typing/gradual: exercism/configlet#842
I actually think that adding both dynamic and static typing tags makes sense too. Maybe for now just omit the gradual and update later?

@ErikSchierboom
Copy link
Member

@ErikSchierboom do you think this PR is ready, or should I add something?

If you (temporarily) remove the typing/gradual tag, I think we're good to go.

@pfertyk
Copy link
Contributor Author

pfertyk commented Dec 12, 2023

I've removed gradual typing and added both static and dynamic ;)

ErikSchierboom
ErikSchierboom approved these changes Dec 12, 2023
View reviewed changes
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Feel free to merge! Congrats on the first exercise

@pfertyk pfertyk merged commit ee7c5ab into main Dec 12, 2023
3 checks passed
@pfertyk pfertyk deleted the setup-hello-world branch December 12, 2023 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BNAndras BNAndras BNAndras left review comments

@ErikSchierboom ErikSchierboom ErikSchierboom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pfertyk @iHiD @BNAndras @ErikSchierboom