Split comment on PR in main.yaml

[KushajveerSingh]

Can we break this line if: failure() && ( steps.clean.outcome == 'failure' || steps.lib.outcome == 'failure' ) into two separate if conditions. Then we can provide two different messages on the PR. In the first message, nbdev_install_git_hooks is missing. In the second nbdev_build_lib is missing.

[hamelsmu]

Hi @KushajveerSingh I am not confident this is a good idea for the following reasons:

• The message will be mostly the same, pointing to the docs on using git hooks and various nbdev cli functions
• The goal of the PR comment is to say "hey something is out of sync, please take a look at the CI logs". The last thing I want is for people to blindly apply CLI commands without first looking at the logs. Also the nbdev logs are quite helpful and are designed to help the user.
• I don't want to try to get into the business of propagating all of the logs onto a PR comment, as that is hard to do properly. I would like to lean on the error messages provided natively by nbdev.
• Many times, failure to install git hooks will result in both of these items failing simultaneously. I would hate for the person to be pinged n times for each separate type failure. Rather, I would prefer to gently alert them that there is an issue and have them inspect the logs for the failing tests. It is usually not common to implement PR comments for failing tests, however given the novelty of nbdev we wanted to provide a reminder to look at the contributing docs where it was obvious that a common mistake is being made.

I hope that background helps. I will leave this issue open for further discussion incase there are other opinions.

[KushajveerSingh]

That makes perfect sense. Thanks, for looking into this.