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

Feedback from API generator change #56

Closed
andrewandante opened this issue Nov 13, 2017 · 20 comments
Closed

Feedback from API generator change #56

andrewandante opened this issue Nov 13, 2017 · 20 comments

Comments

@andrewandante
Copy link
Contributor

andrewandante commented Nov 13, 2017

First, let me say thanks and great job on switching to SAMI so quickly!

However, there has been strong feedback from the community with regards to the new layout of the docs. Below is a summary of the features that are now missing, in a different place or otherwise disliked:

    • The amount of noise from inherited methods and the sheer amount of space it takes up on a page are off-putting and often lead to developers giving up (Too much repeating info  #62)
    • A specific documentation integration - Dash - no longer works
    • A link to the file that contains the class is no longer present - I assume a link to GitHub would be acceptable in this case (Link to Github source code #59)

It seems to me that most of this is templating rather than fundamental changes to the code.

@tractorcow
Copy link

How about a hide inherited members button that toggles these on/off? It could even be off by default, and blocks of inherited members could have a placeholder toggle with click-to-show?

@tractorcow
Copy link

We could fix the github integration link too; It was turned off because Sami doesn't natively support multi-repo docs links (although it does support single-repo links).

@tractorcow
Copy link

More feedback over on framework at silverstripe/silverstripe-framework#7625. I'm happy to leave the framework issue open for visibility.

@chillu
Copy link
Member

chillu commented Nov 26, 2017

@andrewandante Can you recall who raised this? I'm keen to have improvements like this more community driven. Maybe the next time it comes up on Slack, get people to add their username here?

@leomeloxp
Copy link

leomeloxp commented Nov 28, 2017

@chillu I was part of this discussion but not the only one to raise some of these issues...

The one related to Dash was certainly me, and I'm not sure if anyone else is affected by this (at least I didn't anyone on Slack speaking up about it yet). As for the other points, I also agree that they are downsides of the new Docs website, but I wasn't the only one.

That being said I appreciate that the old docs engine doesn't support some of the things that SS4 makes heavy use of which meant that we either have the current docs or no docs at all (at least for the shorter term) so thanks for getting something up and running whilst docs maintainers figure a more robust solution out.

Whilst I don't think I'm able to represent anyone's views but my own. I'm happy to try and assist on whatever I can in order to improve the quality of the docs website. Cheers.

@robbieaverill
Copy link
Contributor

For some context the APIgen library we used to use doesn't have a stable version right now, which was one of the motivators to change to a new framework. We didn't know people use external integration systems at the time. If this is something you're vocal about retaining I'd suggest perhaps getting into the dev for the latest unstable APIGen version and maybe we could look at switching back to it if the community really wants it, otherwise I think we will look at improving UX for the current system (which seems more maintained).

@chillu
Copy link
Member

chillu commented Nov 28, 2017

maybe we could look at switching back to it if the community really wants it

I would only be willing to consider this if there's a strong owner for api.silverstripe.org to maintain the site going forward emerging from the community. The current format is good enough for the most popular PHP packages out there (api.symfony.com), so I really hope that we can make it work on this platform.

@leomeloxp Can you please raise a separate issue about Dash integration, and describe there how it works, maybe also investigating how we could make this happen on the new SAMI API generator?

From SilverStripe Ltd. teams perspective, I don't think we'll be able to invest much time into this over the next month or two. Is there anything the community needs to get started with their own pull requests and improvements?

@leomeloxp
Copy link

@chillu @robbieaverill To be frank this feature is not something I'm extremely passionate about, and unfortunately I haven't had time to look into it in detail yet...

Since it is paid software I'm gonna try and go through the official ways of providing feedback to the developer of the software and their community and see what steps can be achieved on their end to maybe add support to SAMI (as not only SS would benefit from but any other package that uses SAMI).

If there's anything on SS's end that we need to/can do specifically to make the integration work, I'll open an issue then. Cheers 😄

@andrewandante
Copy link
Contributor Author

@chillu I genuinely think that the community is/was under the impression that they couldn't make PRs, so making that more widely known is probably a good start. I understand the thinking that it "belongs" to SilverStripe LTD.

Maybe a Contributing section in the Readme.md would help? Having it there makes it more obvious that contributions are accepted. Guidelines will help too

@chillu
Copy link
Member

chillu commented Nov 30, 2017

@andrewandante Good point, I've added a specific note about community contributions on the repo: 74fca05.

@worikgh
Copy link

worikgh commented Jan 15, 2018

There is no list of derived classes. Looking for instantiations of an Abstract class becomes a chore.

Hop this is a useful place to say that....

@tractorcow
Copy link

That's a good thing to point out. Hope we can add this in.

@tractorcow
Copy link

I'm starting to think maybe we should just switch off Sami onto something else entirely.

@chillu
Copy link
Member

chillu commented Jan 15, 2018

I'm starting to think maybe we should just switch off Sami onto something else entirely.

Only if its largely a community effort :) I maintain that if Sami is good enough for the Symfony and Laravel API docs, it should be good enough for ours. And Sami is an open source project after all, if you're missing features like derived classes they can be added there.

@wernerkrauss
Copy link

I'm missing the variable explanations from the source code. E.g. http://api.silverstripe.org/3/Requirements.html#method_css

The source says:

/**
 * Register the given stylesheet into the list of requirements.
 *
 * @param string $file  The CSS file to load, relative to site root
 * @param string $media Comma-separated list of media types to use in the link tag
 *                      (e.g. 'screen,projector')
 */

New api docs just state

css(string $file, string $media = null)
Register the given stylesheet into the list of requirements.

In this case I'm clearly missing that the $file param is relative to the site root, not to themes folder, which causes troubles to newbees, see https://stackoverflow.com/questions/48523133/silverstripe-requirementscss-does-not-include-css

@chillu
Copy link
Member

chillu commented Jan 31, 2018

It looks to me like the detail view for that method isn't rendered, which should have those parameter level infos. If you scroll to the bottom of the page the method details finish with customCSS(), which is about halfway through. Same problem with the get_custom_scripts() method.

Maybe have a look if the theme is not set up properly? https://github.com/silverstripe/api.silverstripe.org/tree/master/conf/themes/silverstripe. I've had a quick browse through the PHP customisations in src/ and I can't see anything that would influence the list of methods presented there.

Do you know of any other ocurrences? Maybe it chokes on a particular PHP or PHPDoc aspect in this particular file?

@chillu
Copy link
Member

chillu commented Jan 31, 2018

Regarding missing source file support, it seems possible with Sami - anybody keen to give it a crack? https://github.com/FriendsOfPHP/Sami/issues/293

@tractorcow
Copy link

It's possible but not with multiple-repos the way we've implemented it. you'd need to re-implement this feature.

@williamdes
Copy link
Contributor

/close @chillu ?

@chillu
Copy link
Member

chillu commented Dec 17, 2020

Yep, given that three out of the four issues raised by Andrew are already tracked as (more focused) separate issues, I'm happy to close this - hasn't gotten traction in three years after all.

Werner's issue with the lack of method param docs due to a "cut off" file still exists after the Sami to Doctum switch, but it seems to be an edge case.

@chillu chillu closed this as completed Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants