Skip to content

WIP: Add SQFLint to Travis#5468

Closed
jonpas wants to merge 4 commits into
masterfrom
travisSQFlint
Closed

WIP: Add SQFLint to Travis#5468
jonpas wants to merge 4 commits into
masterfrom
travisSQFlint

Conversation

@jonpas
Copy link
Copy Markdown
Member

@jonpas jonpas commented Aug 30, 2017

When merged this pull request will:

  • Title

Todo:

  • Make it work without crashing (possibly needs waiting for SQFLint updates)
  • Return an error code for warnings (warnings-as-errors) for Travis
  • Run only on changed .sqf files

@jonpas jonpas added this to the Ongoing milestone Aug 30, 2017
@kymckay
Copy link
Copy Markdown
Member

kymckay commented Aug 30, 2017

Is there no way to run for only the changed files?

@jonpas
Copy link
Copy Markdown
Member Author

jonpas commented Aug 30, 2017

@silentspike As said in OP:

Run only when the diff includes an .sqf file change (otherwise build times will go up significantly for other PRs as well)

So yes, there is a way by diffing and pulling out file name extensions, have done it before. Problem is when some non-SQF file might impact building success when compiling, but for a linter like this that can't happen.

@kymckay
Copy link
Copy Markdown
Member

kymckay commented Aug 31, 2017

I meant more, it looks like it's running for the entire addons folder: sqflint -d addons, is there no way to run the linter over only the changed files?

@jonpas
Copy link
Copy Markdown
Member Author

jonpas commented Aug 31, 2017

Ah, yes, good idea!

@LordGolias
Copy link
Copy Markdown
Contributor

Let me know if you need help here. I can modify SQFLint so that it works well here.

@jonpas
Copy link
Copy Markdown
Member Author

jonpas commented Sep 3, 2017

@LordGolias Right now it crashes as seen here.

@LordGolias
Copy link
Copy Markdown
Contributor

LordGolias commented Sep 3, 2017

You are right. I just pushed a fix of that to master of sqf. I also updated the pip version so that you can install the latest using pip install.

I checked that the ACE addons is analysed without any further crash.

@jonpas
Copy link
Copy Markdown
Member Author

jonpas commented Sep 3, 2017

@LordGolias Very nice! Travis ran to the end now.

Next step is making it return an error code instead. I am not sure this should be configurable somehow as some might not want warnings reported as errors.

@jonpas
Copy link
Copy Markdown
Member Author

jonpas commented Sep 3, 2017

@silentspike We might still want to run full lint on merges into master, just to identify anything that may get through in whatever way, maybe?

@kymckay
Copy link
Copy Markdown
Member

kymckay commented Dec 10, 2017

I think it would be unnecessary unless the linter is updated in a way that it cross-examines function calls for return value types or something. Might not be a bad idea to run it fully for release branch though?

@thojkooi
Copy link
Copy Markdown
Contributor

Why not run it for every file? Does the linting take a long time to complete?

@kymckay
Copy link
Copy Markdown
Member

kymckay commented Dec 10, 2017

Nah not necessarily, not sure how it would scale if being run for every commit though. I'm committing the sin of premature optimization 😆

@jonpas
Copy link
Copy Markdown
Member Author

jonpas commented Dec 10, 2017

I recommend:

  • Changed files lint on PR/branch commits (I believe that's what @thojkooi meant?)
  • Full lint on merges to master and release

Full lint does take a bit, we have A LOT of files.

@thojkooi
Copy link
Copy Markdown
Contributor

But is it an issue? How long does a full lint take?

We almost never merge anything straight away. Just lint everything is my suggestion.

@jonpas
Copy link
Copy Markdown
Member Author

jonpas commented Dec 10, 2017

real    1m19.454s
user    0m57.734s
sys     0m0.922s

@thojkooi
Copy link
Copy Markdown
Contributor

I'd just run the full lint. We almost never merge anything that fast anyway.

@thojkooi thojkooi changed the title Add SQFLint to Travis WIP: Add SQFLint to Travis Nov 18, 2018
@thojkooi thojkooi mentioned this pull request Dec 2, 2018
@thojkooi thojkooi closed this Dec 2, 2018
@jonpas jonpas deleted the travisSQFlint branch December 18, 2019 17:35
@LinkIsGrim LinkIsGrim removed this from the Ongoing milestone Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants