AllenInstitute / MIES

Multichannel Igor Electrophysiology Suite
https://alleninstitute.github.io/MIES/user.html
Other
21 stars 6 forks source link

CI: Add ipt format to check formatting for PRs #2077

Closed Garados007 closed 3 months ago

Garados007 commented 3 months ago

Thanks for opening a PR in MIES :sparkles:!

t-b commented 3 months ago

Looks mostly good. But I think we only want to format files matching *MIES*.ipf from Packages/MIES and not the other files in that folder.

Garados007 commented 3 months ago

Fixed

t-b commented 3 months ago

@Garados007 Sorry for being unclear. We want to format all ipf files in the repo (without submodules) except files from Packages/MIES which don't match *MIES*.ipf.

t-b commented 3 months ago

I would also prefer to have the script do the formatting directly without the config.toml. Yes that does not scale for arbitrary number of files but works right now. Something like:

ipt format -i $(git ls-files 'Packages/MIES/*MIES*.ipf' 'Packages/tests/**/*.ipf' 'Packages/doc/**/*.ipf' 'Packages/Conversion/**/*.ipf')
t-b commented 3 months ago

I've pushed a modification. We are now tagging all files we want to format with the ipt attribute and the script then formats these files. How about that?

Garados007 commented 3 months ago

There was a reason why I used the config file instead of providing them as arguments. In Windows can be the argument line only up to 8191 characters (source) and if the number of files increases it is more likely to hit this limit. On the other hand the config file allows unlimited number of entries and is therefore future proof.

Garados007 commented 3 months ago

I checked the call in your script and it is right now at 7963 characters. 3 files more and the script does not work anymore.

Garados007 commented 3 months ago

I found other sources saying the limit is 32767 characters. It is a bit higher but not impossible to reach in the foreseeable future.

Garados007 commented 3 months ago

But I do like the idea about the git tags.

t-b commented 3 months ago

We are not using CMD.exe but a bash script so the limit is around 32k, which you already mentioned, which is nearly four times as many files as currently or?

If you insist we can still create a config file from the output of git ls-files .... But in that case no need to bother with cleaning up in CI of the config.toml.

t-b commented 3 months ago

But I do like the idea about the git tags.

It's attributes, but yes I think we should keep that.

Garados007 commented 3 months ago

@t-b Are you okay with my changes? I reintroduced the config file, because I think its better to do so.

t-b commented 3 months ago

@Garados007 Jeep.

Garados007 commented 3 months ago

I rebased all the stuff.

t-b commented 3 months ago

@Garados007 Thanks. LGTM.