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

GDScript: Add module description in markdown #81345

Merged
merged 1 commit into from
Jan 9, 2024

Conversation

anvilfolk
Copy link
Contributor

@anvilfolk anvilfolk commented Sep 5, 2023

This adds a .md file to the GDScript module that gives a moderately in-depth description of how the module is organized, used, main classes, etc :)

@anvilfolk anvilfolk requested a review from a team as a code owner September 5, 2023 14:57
@YuriSizov YuriSizov added this to the 4.2 milestone Sep 5, 2023
@akien-mga akien-mga changed the title GDScript: add module description in markdown GDScript: Add module description in markdown Sep 5, 2023
@anvilfolk anvilfolk force-pushed the gdoverview branch 2 times, most recently from 511c2e2 to 6370f43 Compare September 5, 2023 17:22
@anvilfolk anvilfolk force-pushed the gdoverview branch 2 times, most recently from a128e07 to 3f490fb Compare September 8, 2023 19:11
Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Great job! I didn’t know much of this or wasn’t completely sure. I've added some suggestions, you don't have to follow them all, these are just ideas that might inspire you.

The README is intended for new contributors, so it's quite difficult to decide which details are important to understand the module and which will require learning the code anyway, so it's not worth the effort.

In my opinion, a section about tests would help new contributors. There is a doc page, but it is mainly about doctest, not about GDScript tests. It would be nice to briefly explain that tests should be positive and negative, how our files are located (folders, *.gd, *.notest.gd, *.out), how to run tests locally.

I hope Adam, Dmitry and George will add their suggestions too.

modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Show resolved Hide resolved
modules/gdscript/README.md Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Show resolved Hide resolved
modules/gdscript/README.md Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
@anvilfolk
Copy link
Contributor Author

@dalexeev excellent comments! I will try to find some time soon to incorporate them! I think a section about tests makes a lot of sense, and we can link to the official docs about it!

@anvilfolk anvilfolk force-pushed the gdoverview branch 2 times, most recently from 7729e01 to b8de8a4 Compare October 6, 2023 13:03
@anvilfolk
Copy link
Contributor Author

Incorporated all of @dalexeev's suggestions! They were so helpful and relevant! 🥰

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Amazing write-up, I learned a lot :)

modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
modules/gdscript/README.md Outdated Show resolved Hide resolved
@anvilfolk
Copy link
Contributor Author

Amazing write-up, I learned a lot :)

Thanks, it's all I'm capable of doing at the moment, but it sounds like folks might find it helpful, so that feels nice :)

It might eventually make sense to add a section on how unit tests are run, but that requires digging more deeply into that code, which I haven't done in a while :)

@anvilfolk
Copy link
Contributor Author

Spent a few more hours on this today. I think it's good to merge if folks don't find anything else they'd like changed.

Changelog:

  • Fixed some references to _populate_class_members() which no longer exists and has been replaced with _prepare_compilation()
  • Added a lot more information and examples to the analyzer, since it had very, very little information
  • Added more information about how cyclic dependencies are resolved
  • Added some information about how typing leads to optimizations during compilation
  • Tiny edits here and there

I think it's helpful for this document to describe a few of the things the analyzer actually does. This means that new contributors don't need to work through a couple hundred lines of code to try to figure it out - they can read it and get a general idea for what it does, and then read the code with more context in mind.

@akien-mga akien-mga modified the milestones: 4.2, 4.3 Oct 26, 2023
@adamscott adamscott mentioned this pull request Dec 4, 2023
27 tasks
@Mickeon
Copy link
Contributor

Mickeon commented Jan 8, 2024

May I poke this? It looks great to me. Would be nice to have sooner than later for the developers

Copy link
Member

@dalexeev dalexeev left a comment

Choose a reason for hiding this comment

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

Looks good to me. Of course, there is always more to add, but we are not trying to write exhaustive internal documentation. This is a good overview for new contributors, and we can edit it later anyway (unless @vnen, @adamscott, @vonagam, and other contributors have suggestions right now). @anvilfolk Thank you again for the great write-up!

@akien-mga akien-mga merged commit 1edcf9a into godotengine:master Jan 9, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks! Great writeup :)

@anvilfolk anvilfolk deleted the gdoverview branch January 9, 2024 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants