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

DOC Update contributing code docs #379

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Oct 18, 2023

  • Move note about docs to "before working on a fix" since everything until then is relevant for docs too
  • Remove note that "Every file view on GitHub has an 'edit this file" link.' because that's confusing at best
  • Move definition of public API to its own page and refer to it always as "public API", not just "API"
  • Updated and clarified definition of public API as it pertains to configuration values/structure
  • fix a link

Issue

* public and protected scope (including methods, properties and constants)
* private static class properties (considered to be configuration variables)
* yml configuration file structure and value types
* extension hooks (e.g. `$this->extend('someExtensionHook'));`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* extension hooks (e.g. `$this->extend('someExtensionHook'));`)
* extension hooks (e.g. `$this->extend('someExtensionHook', $someVariable);`)

Copy link
Member Author

Choose a reason for hiding this comment

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

You can absolutely create valid and useful extension hooks that don't pass arguments - and this isn't documentation about how to create or use extension hooks. But I'll make this change to avoid peer review ping pong since I have to remove that extra bracket anyway.

Done.


* private scope (with the exception for `private static` properties which aren't annotated with `@internal`)
* all entities marked as `@internal`
* yml configuration file default values
Copy link
Member

@emteknetnz emteknetnz Oct 24, 2023

Choose a reason for hiding this comment

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

There's no difference between yml config and private static config - this reads like yml config is out of scope though private static class properties are in scope (it's listed above). Need to clarify

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was just copying what was already there and moving it to this page, but if you're saying updating this is in scope then I'll update it.

Done.

* namespaces, classes, interfaces, enums, and traits
* public and protected scope (including methods, properties and constants)
* private static class properties (considered to be configuration variables)
* yml configuration file structure and value types
Copy link
Member

@emteknetnz emteknetnz Oct 24, 2023

Choose a reason for hiding this comment

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

what is "yml configuration file structure"? And what is a yml "value type"?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was existing wording... I've done my best to clarify it now to what I think was intended.

@GuySartorelli GuySartorelli force-pushed the pulls/4.13/update-contributing-code branch from 7634e9a to 5e19111 Compare October 24, 2023 20:58
- Move note about docs to "before working on a fix" since everything until then is relevant for docs too
- Remove note that "Every file view on GitHub has an 'edit this file" link.' because that's confusing at best
- Move definition of public API to its own page and refer to it always as "public API", not just "API"
- fix a link
@GuySartorelli GuySartorelli force-pushed the pulls/4.13/update-contributing-code branch from 5e19111 to 7e9499f Compare October 24, 2023 21:36
@emteknetnz emteknetnz merged commit 8566c1f into silverstripe:4.13 Oct 24, 2023
@emteknetnz emteknetnz deleted the pulls/4.13/update-contributing-code branch October 24, 2023 23:30
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