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 a GitHub action to lint phpdocs #9806

Closed
wants to merge 1 commit into from
Closed

Add a GitHub action to lint phpdocs #9806

wants to merge 1 commit into from

Conversation

williamdes
Copy link
Contributor

Add a GitHub action to lint phpdocs

@williamdes
Copy link
Contributor Author

@sminnee
Copy link
Member

sminnee commented Dec 22, 2020

Nice work!

A few comments:

  • the action is nice & quick, the core "build" step is only 6 seconds, so this seems a good price to pay for keeping the code compliant with api doc generation - especially if we were to introduce other things like phpstan lining that might inadvertently break it

  • why, however, are using the "ignore parse errors" option? Surely that's exactly what we would want to pick up?

  • more a question for the core team - is there anything we need to do to make this more efficiently shared across the various modules that have their docs on api.SS.org?

@williamdes
Copy link
Contributor Author

Thank you !

why, however, are using the "ignore parse errors" option? Surely that's exactly what we would want to pick up?

There is errors I can not fix, this is to make the action pass but still output errors, for example on pull-requests
More a warning more until all errors are removed

@williamdes
Copy link
Contributor Author

cc @chillu

@chillu
Copy link
Member

chillu commented Jan 13, 2021

Thank you William! I would rather have this in the Travis config, since it'll be far easier to roll out across our large number of modules - see #9174. But then again, this is a great starting point - and we're only running a handful of modules where PHPDoc linting is important (those going on api.ss.org)

@steve-silverstripe @dnsl48 Since you worked on the shared Travis config stuff, keen on your thoughts.

@dnsl48
Copy link
Contributor

dnsl48 commented Jan 13, 2021

The guts of it is the beautiful github action sudo-bot/action-doctum, which uses the docker container underneath:

runs:
    using: "docker"
    image: "docker://botsudo/action-doctum:latest"
    args:
        - latest
        - "${{ inputs.config-file }}"
        - "${{ inputs.method }}"
        - "${{ inputs.cli-args }}"

I guess we could run that container in our Travis shared configs. That would allow us to easily scale that lint to the hundred modules. With GitHub actions that would require us to copy-paste this PR and make a hundred others - separately for each repository.

Another potential problem is - GitHub Actions are out of sight for the maintainers. E.g. we won't remember to update the doctum action version. Now it's sudo-bot/action-doctum@v5, but when v6 gets released - we may have to do a hundred PRs to update it everywhere and wouldn't remember to actually do that.

@emteknetnz
Copy link
Member

emteknetnz commented Jan 14, 2021

+10 for try to get this into travis shared config rather than github actions
https://github.com/silverstripe/silverstripe-travis-shared

@williamdes
Copy link
Contributor Author

+10 for try to get this into travis shared config rather than github actions
https://github.com/silverstripe/silverstripe-travis-shared

I will keep that in mind, but this will have some delay before I fix that :)

@williamdes
Copy link
Contributor Author

I can assist to create the script but I am too far away from this organization to understand well how the shared repo works.
But ping me on the PR that will add it

@williamdes williamdes closed this Apr 10, 2021
@williamdes williamdes deleted the doctum-action branch April 10, 2021 21:50
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.

None yet

5 participants