CaseyHofland / docfx-unitypackage

DocFX Unity package is a GitHub action for deploying a DocFX website for Unity packages to GitHub Pages.
https://github.com/marketplace/actions/docfx-unitypackage
MIT License
21 stars 8 forks source link

ability to specify path to packge.json file #1

Closed StephenHodgson closed 1 year ago

StephenHodgson commented 1 year ago

It'd be great to be able to specify the path to the package.json file to build docs for.

CaseyHofland commented 1 year ago

docfx-unitypackage is meant to be used on unity packages hosted on github. This means that the package.json will always be at the root. Do you have a repository where this isn't the case and if so would you share it with me so I can take a closer look?

StephenHodgson commented 1 year ago

If you add it as an optional parameter it makes the action a lot more useful to me, yes

StephenHodgson commented 1 year ago

Do you have a repository where this isn't the case and if so would you share it with me so I can take a closer look?

https://github.com/RageAgainstThePixel/com.openai.unity

CaseyHofland commented 1 year ago

In your case there's actually a cleaner way, which is to target the #upm branch instead of the #main branch.

Changing the on: line in the workflow should do it:

on:
  workflow_dispatch:
  push:
    branches:
      - upm
StephenHodgson commented 1 year ago

I understand but I'm trying to make it more flexible for other people's projects as well. It should be pretty simple to specify a relative path or set the default path.

StephenHodgson commented 1 year ago

If you want I don't mind opening a pull request

CaseyHofland commented 1 year ago

It is pretty simple to support, but I don't know if it should. To my knowledge, in order to share Unity packages over GitHub you need a repo conforming to the package layout.

If the action allows specifying a path, then it is deviating from this requirement. I don't want to make it seem like you should be able to host a package from a subfolder until I find a good example where this deviation makes sense.

But I really appreciate the offer to make a pull request for this! It's really cool to me that you show so much interest about this project. On that note, you make some pretty cool stuff as well 💥

StephenHodgson commented 1 year ago

If the action allows specifying a path, then it is deviating from this requirement

What requirement?

CaseyHofland commented 1 year ago

In order to share Unity packages over GitHub you need a repo conforming to the package layout.

The link shows the required layout for Unity packages, and if you want to host them via github you need at least 1 branch to conform to it.

StephenHodgson commented 1 year ago

I don't believe this is a hard requirement since most/all my packages do this in a dedicated upm branch.

I don't particularly want to fork as you've done such an amazing job getting it this far, and it is a relatively low lift item. Could we reopen this and I offer a pull request?

It would not change the default behavior of the task 🙏

CaseyHofland commented 1 year ago

But it's already supported. You just need to target your upm branch, as shown a few posts above.

(Thanks btw <3)

StephenHodgson commented 1 year ago

okay, I will try it out this way. Typically I don't like to have actions trigger from alternate branch pushes, but I'll tinker and see how it goes 🚀

I was hoping to incorporate it into the existing pipelines I have for building phases that do automated tested with an editor all in one go.

CaseyHofland commented 1 year ago

I don't want to make it seem like you should be able to host a package from a subfolder until I find a good example where this deviation makes sense.

I don't want to allow features that support bad practices.

StephenHodgson commented 1 year ago

Mainly I check in the editor project as well so I can do automated testing, building examples and vetting the package.

I don't want to allow features that support bad practices.

Funny enough I feel as though putting everything as just a package in the root of a repo is bad practice, bc there's no way to vet the changes and build them locally before publishing 🙈

CaseyHofland commented 1 year ago

I was hoping to incorporate it into the existing pipelines I have for building phases that do automated tested with an editor all in one go.

It shouldn't conflict with that, but if you're talking about stopping the docs from building in case an error occurs then I see how that could be a problem.

Perhaps a better solution would be to specify a branch to build the docs from instead: would that solve your issue?

CaseyHofland commented 1 year ago

Funny enough I feel as though putting everything as just a package in the root of a repo is bad practice, bc there's no way to vet the changes and build them locally before publishing 🙈

I don't disagree but it's Unity's layout I'm following so concessions gotta be made 🥲

StephenHodgson commented 1 year ago

The layout is being applied and enforced, it's just not at the root level. I don't think there's any hard requirement about a project having to be at the root level to be a valid package. It hasn't stopped people from using my plugins in the past.

CaseyHofland commented 1 year ago

I've taken a closer look at your workflows and I really believe the best solution is to add this script as an additional workflow:

name: docfx-unitypackage

on:
  workflow_dispatch:
  push:
    branches:
      - upm

jobs:
  build-and-deploy:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout
        uses: actions/checkout@v3
        with:
          submodules: true

      - name: Build
        uses: CaseyHofland/docfx-unitypackage@v1
        with:
          github_token: ${{ secrets.GITHUB_TOKEN }}

      - name: Deploy
        uses: peaceiris/actions-gh-pages@v3
        with:
          github_token: ${{ secrets.GITHUB_TOKEN }}
          publish_branch: gh-pages
          publish_dir: _site

The following should cover all your needs. Since you have a workflow that runs on main and pushes to upm, this action will run automatically after that. Even better, should you ever have a reason to push to upm directly, this action will automatically handle that case as well.

I will no longer comment on this issue but I hope this solves your problem, I really believe it is the best solution.