silverstripe / api.silverstripe.org

API documentation for the Silverstripe Framework
BSD 3-Clause "New" or "Revised" License
3 stars 14 forks source link

NEW Include properties tagged as `@config` #75

Closed dhensby closed 6 years ago

dhensby commented 6 years ago

Fixes #74

Adds private static vars (and others) that are marked as @config.

2018-06-26_14-33-41_6565efc4-4e3e-4b3e-ac51-e9d720744c37

Further optional enhancements:

I'm currently looking into grouping configs separately to properties, I'm 50/50 on whether it's going to be possible it's done :)

dhensby commented 6 years ago

Added "config options" section :)

2018-06-26_17-38-47_57e529e9-408b-4d48-a182-0f22748d78ea

dhensby commented 6 years ago

I guess the only question left is should we include all private static variables or just the ones marked as @config?

robbieaverill commented 6 years ago

I guess the only question left is should we include all private static variables or just the ones marked as @config?

I think the ones marked as config would be fine. It's a practice we're trying to encourage right, even though any private static will work as a config prop

dhensby commented 6 years ago

I think the ones marked as config would be fine. It's a practice we're trying to encourage right, even though any private static will work as a config prop

I'm concerned that in 3.x we are not so good at tagging vars as @config...

robbieaverill commented 6 years ago

True. To be fair, I guess the cases where we're using private statics that aren't supposed to be config props would be lower than the cases of private statics that are but don't have the tag...

dhensby commented 6 years ago

Agreed - ok, well this could be merged as is and I'll add another PR to add all private static properties or I'll add it in my tomorrow to this PR

robbieaverill commented 6 years ago

Will deploy to UAT now

sminnee commented 6 years ago

@dhensby I would recommend instead that it would be best to force that we mark things as @config rather than picking up all private statics.

If we want a compromise, I would suggest that we show all private statics only for our SS3 docs, and not for SS4.


The other thing I wanted to flag is whether we should document these as config options, rather than presenting them as private statics?

I'm not sure what that would look like but you could for example show an example yaml content, as that is how people will typically set these, or just a table that excluded the private static boilerplate with is more confusing than helpful IMO.


Config options

SilverStripe\Assets\File:
  singular_name: "string" 
  non_live_permissions: [ ... ] # Anyone with CMS access can view draft files
  allowed_extension: [ ... ]

Config options in SilverStripe\Assets\File

Config option Type Description
singular_name string
non_live_permissions array Anyone with CMS access can view draft files
sminnee commented 6 years ago

I forgot to say – it's awesome that our API generation tool has this level of flexibility and this is a great step forward!

dhensby commented 6 years ago

If we want a compromise, I would suggest that we show all private statics only for our SS3 docs, and not for SS4.

I thought about this, but there's no way (that I'm aware of at the moment) for the "filter" to know what version of code we are analysing. I'm happy to leave it as-is and not add all private static properties to config options

dhensby commented 6 years ago

The other thing I wanted to flag is whether we should document these as config options, rather than presenting them as private statics?

This is a good point, providing them as YAML is going to be a bit too much effort for me to justify right now, I think. especially as the type is not always known. I'll make a PR to remove the private static part.

One glaring issue with this is that the config options defined in YAML only are not presented here.

Do we want to show the default value for the options too? (if that's possible)

dhensby commented 6 years ago

I forgot to say – it's awesome that our API generation tool has this level of flexibility and this is a great step forward!

I credit my awesome hacking skills over how flexible the generator is ;) These changes are a bit risky as it's involved copy and pasting of parent methods in some instances... I felt it better to have slightly less stable docs code and better docs than slightly more stable docs code and less helpful docs