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

Add new template for running yii2-apidoc on a user-submitted project #151

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jcherniak
Copy link

Q A
Is bugfix? no
New feature? yes
Breaks BC? no
Tests pass? yes
Fixed issues #131

@ricpelo
Copy link
Contributor

ricpelo commented Nov 6, 2017

It works for me!

@samdark samdark requested a review from cebe November 6, 2017 21:56
/**
* @var string URL for the README to use for the index of the guide.
*/
public $readmeUrl = "https://raw.github.com/yiisoft/yii2-framework/master/README.md";
Copy link
Member

Choose a reason for hiding this comment

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

Why URL instead of local file path?

Copy link
Author

Choose a reason for hiding this comment

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

That URL was hard-coded in the bootstrap template which I copied. For it to work without changing the command line params, it has to match the value that was in the previous template.

$appTypes = $this->filterTypes($types, 'app');

// It's a hack, but we'll go with it for now.
$readme = @file_get_contents($this->readmeUrl);
Copy link
Member

Choose a reason for hiding this comment

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

Silencing errors isn't good.

Copy link
Author

Choose a reason for hiding this comment

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

I suppose I could throw an exception here. Would that be preferred?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for the record, Bootstrap template silences errors, too:

$readme = @file_get_contents("https://raw.github.com/yiisoft/yii2-framework/master/README.md");

@ricpelo
Copy link
Contributor

ricpelo commented Nov 6, 2017

It works good, but IMHO it isn't a perfect solution. The navigation sidebar never displays app\* and yii\* classes at the same time. It could be great if all the classes of app and yii namespaces were displayed together in the navigation sidebar.

@jcherniak
Copy link
Author

@ricpelo - Do you want yii namespaces in a custom project? I'd think not as it would make the sidebar be very crowded. I used this to generate docs for Craft CMS 3, which already has a ton of namespaces and classes. I'd think you wouldn't want the yii classes there.

@ricpelo
Copy link
Contributor

ricpelo commented Nov 6, 2017

@jcherniak - First of all, thank you for your code! It works for me, as I said before. Maybe you're right: I only tested with a-few-classes project but with tons of classes could be overkill to have both app and yii namespaces at the same time.

@samdark samdark added the type:enhancement Enhancement label Nov 7, 2017
@cebe cebe added this to the 2.0.7 milestone Nov 13, 2017
@ricpelo
Copy link
Contributor

ricpelo commented Nov 28, 2017

Hi! Will this PR be merged for 2.0.7 release (or maybe before :))? Thanks!

@jcherniak
Copy link
Author

Any chance this can be merged?

@ricpelo
Copy link
Contributor

ricpelo commented Feb 22, 2018

Yes, please. I've used it for months and it works great, IMO.

@samdark
Copy link
Member

samdark commented Feb 22, 2018

We need @cebe here.

@jcherniak
Copy link
Author

Any chance this can be merged now? Looks like @cebe made a few changes, so it would be nice to get this into core.

@samdark samdark removed this from the 2.0.7 milestone Feb 15, 2019
@arogachev arogachev self-requested a review November 25, 2021 05:16
@arogachev arogachev self-assigned this Nov 25, 2021
@mtangoo
Copy link
Contributor

mtangoo commented Aug 3, 2023

@cebe @samdark @arogachev is this PR going to be merged or should it be closed?

@samdark
Copy link
Member

samdark commented Aug 18, 2023

@mtangoo I'd get this one in. Would be great if you'll test it with custom project.

@mtangoo
Copy link
Contributor

mtangoo commented Aug 18, 2023

I will find time and test it. Wanted to be sure there's desire to merge it in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants