Skip to content

Conversation

@DaAwesomeP
Copy link
Member

Working toward #1815. This is still a draft. This pull is a WIP. It should not be merged until after #1817 is merged.

I have so far put both spelling tasks in one script, as they both require the same dynamically generated file list.

I will probably try Doxygen next.

@DaAwesomeP
Copy link
Member Author

@peternewman spellintian complains of a lot of speeling errors. Have these always been present but the CI has just been ignored, or are these appearing for the first time? I am able them reproduce it locally.

@peternewman
Copy link
Member

I will probably try Doxygen next.

👍 but doing it in another PR probably makes sense so this one doesn't get stuck!

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

A few initial comments

@peternewman
Copy link
Member

@peternewman spellintian complains of a lot of speeling errors. Have these always been present but the CI has just been ignored, or are these appearing for the first time? I am able them reproduce it locally.

Yeah the spelllintian stuff has been allowed failures historically:
https://app.travis-ci.com/github/OpenLightingProject/ola/builds/259318060

Unfortunately Travis seems to have cycled the old logs now...

As there was no easy way to ignore stuff I just used to manually check them occasionally.

@DaAwesomeP
Copy link
Member Author

DaAwesomeP commented Feb 20, 2023

Yeah the spelllintian stuff has been allowed failures historically:
https://app.travis-ci.com/github/OpenLightingProject/ola/builds/259318060

OK, I will make spellintian generate warnings instead of errors then and will make it always pass (unless the linter fails to run of course).

@DaAwesomeP DaAwesomeP changed the title GitHub Actions Lint: spelling GitHub Actions Lint: spellintian, codespell Feb 20, 2023
@DaAwesomeP
Copy link
Member Author

DaAwesomeP commented Feb 20, 2023

OK, I made spellintian always pass for now and it generates warning annotations instead of errors. Unfortunately GitHub Actions has a continue-on-failure option for specific steps, but it will still mark the workflow as failed.

Codespell seems to be grumpy about a compiled .cc file. I guess it shouldn't include those in its file list? Fixed!

@DaAwesomeP DaAwesomeP marked this pull request as ready for review February 20, 2023 16:20
@peternewman
Copy link
Member

Can you resync please @DaAwesomeP to reduce the diff here. I can hit the button but I figured it you might find it easier if you do it directly your end.

@DaAwesomeP
Copy link
Member Author

@peternewman Try reloading? I see only 3 files changed.

@peternewman
Copy link
Member

Doh, of course it's just done it!

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

A few comments

@DaAwesomeP
Copy link
Member Author

@peternewman OK, I think this one is good to go!

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Just some very minor nits to tidy up, great that it's all working now!

@@ -0,0 +1,99 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

As sort of mentioned before, I'm beginning to wonder more if this should all become ci.sh or part of the Makefile or something, I'm not really sure. Probably makes sense to leave it until everything is migrated so we can easily re-assess what it all looks like compared to .travis-ci.sh .

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I have some ideas for wrapping things but I just want to get everything working initially first. I'm significantly changing things such that I would write a new ci.sh from scratch probably. The current one is very Travis-centric.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's cool, lets leave this for the future then.

Copy link
Member

@peternewman peternewman left a comment

Choose a reason for hiding this comment

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

Great thanks @DaAwesomeP !

@peternewman peternewman merged commit 74f80b2 into OpenLightingProject:0.10 Feb 24, 2023
@peternewman peternewman added this to the 0.10.10 milestone Mar 2, 2024
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.

2 participants