Adding Perl installation + pre-commit hook #322
Conversation
|
Thanks for this. All pull requests should go to If this only works for conda, and it relies on an issue that isn't yet fixed, then I don't think I can merge this. I don't have any updates on the GCString issue, all updates will be posted to that thread. |
|
On conda it should work on everything but macOS ARM. I'm fine with waiting for #303 . Or, if you think it's sufficiently safe, I could also apply the non-GCString version as patch to the conda package. Alternatively, there is the |
|
OK, many thanks. Can we see if I can progress #303? It's one of my next priorities, and I'm hopeful to look at it soon. |
|
A few questions on this, if I may:
|
|
I have added documentation to answer part of your questions.
That is relatively straightforward.
For it to work you will need a conda installation, and you will need to have
This is repository file. As a tool provider you provide some rules for
Under the
It allows Let me know if there are other questions. What I would have liked: a GitHub Action to check all of this continuously. I don't know how to do this though. One could have used the local run as outlined under 1. But it seems crucial to use two repos there. Also, it seems to be using the latest release, which is sort of ok, but not ideal for continuous integration. |
|
Thanks that's helpful. I'll review it soon. In the meantime, it's the documentation / |
|
I have tried on my Linux machine:
|
|
OK, many thanks. Looks like this has diverged from develop, it'd be great if you could rebase. |
|
I rebased and pushed, but somehow GitHub is unhappy about a conflict...? Maybe you can fix it? |
|
OK. But, as I review, it'll need the perl/cpan version to work. I don't use conda, and I don't require users to use it, so having a feature that only works with it is not an option. I hope to get to this soon.... |
|
If you'd just find a solution for the |
|
Hello both and thank you @tdegeus for taking over the implementation of #316
According to my tests, no lookups are made for defaultSettings.yaml file in the LatexIndent directory. I found path lookups to be not easy to grasp (Perhaps because the project has to be compliant with the texlive distribution). Anyway, I tested your changes with copying the defaultSettings.yaml file in the LatexIndent directory and making the following change would solve the problem. --- a/LatexIndent/GetYamlSettings.pm
+++ b/LatexIndent/GetYamlSettings.pm
@@ -61,6 +61,8 @@ sub yaml_read_settings{
# grab the logger object
$logger->info("*YAML settings read: defaultSettings.yaml");
$logger->info("Reading defaultSettings.yaml from $FindBin::RealBin/defaultSettings.yaml");
+ my $dirName = dirname(__FILE__);
# if latexindent.exe is invoked from TeXLive, then defaultSettings.yaml won't be in
# the same directory as it; we need to navigate to it
@@ -71,6 +73,8 @@ sub yaml_read_settings{
$defaultSettings = YAML::Tiny->read( "$FindBin::RealBin/../../texmf-dist/scripts/latexindent/defaultSettings.yaml");
} elsif ( -e "$FindBin::RealBin/LatexIndent/defaultSettings.yaml" ) {
$defaultSettings = YAML::Tiny->read( "$FindBin::RealBin/LatexIndent/defaultSettings.yaml");
+ } elsif ( -e "$dirName/defaultSettings.yaml" ) {
+ $defaultSettings = YAML::Tiny->read( "$dirName/defaultSettings.yaml");
} else {
$logger->fatal("*Could not open defaultSettings.yaml");
$self->output_logfile();
I agree, conda may be a useful option for some users but it should not be a requirement. I think the docs should reflect this idea by mentioning the cpan version of the hook first (or recommending the use of cpan?). |
|
@hamdanal I think it is a good idea to make the cpan the default (it is probably also slightly faster on initial setup), and I would even using as naming |
|
A remark though: in the conda package I do place the Edit So if this indeed possible there is thus the question how to make |
I agree. The new naming makes more sense.
Unfortunately I don't have any experience with perl at all so this is beyond what I can help with. I would note however that the change I made was enough to install the package with the settings file included (maybe this is the default behaviour?)
It appears that you copy everything to a bin directory |
|
Thanks both. I'm a little worried about the defaultSettings.yaml issue, as it seems to be occurring for others too #323 I'm hopeful to review all of this soon. |
|
In the coda package I indeed copy My thoughts for @cmhughes I feel that you might have |
|
Let's assume that |
|
I completely agree the not side tracking, but I could not resist ;) |
|
Here is what I've done so far on my Ubuntu machine: Then I've added repos:
- repo: https://github.com/tdegeus/latexindent.pl
rev: 3.13.5
hooks:
- id: latexindent.pl
args: []Then I've run This gives me Now I need to dig into the error. The pre-commit.logAn error has occurred: InvalidManifestError: =====> /tmp/tmpdbvpsgji/repok44nbqni/.pre-commit-hooks.yaml does not exist Traceback (most recent call last): File "/snap/pre-commit/51/lib/python3.5/site-packages/cfgv.py", line 46, in reraise_as yield File "/snap/pre-commit/51/lib/python3.5/site-packages/cfgv.py", line 413, in load_from_filename raise ValidationError('{} does not exist'.format(filename)) cfgv.ValidationError: =====> /tmp/tmpdbvpsgji/repok44nbqni/.pre-commit-hooks.yaml does not existDuring handling of the above exception, another exception occurred: |
|
When you do
|
|
P.S: I think it is better if you install pre-commit with pip instead of snap: |
|
Many thanks, that's helpful. Here's what I've done: I then updated my I then ran Then running gives ===============================================================================
Using config:
===============================================================================
repos:
- repo: ../../projects/tdegues-latexindent
rev: 149a22c6251db129dbac767efc3d0541b6a8991c
hooks:
- id: latexindent-cpan
===============================================================================
[INFO] Initializing environment for ../../projects/tdegues-latexindent.
[INFO] Installing environment for ../../projects/tdegues-latexindent.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
latexindent.pl...........................................................Failed
- hook id: latexindent-cpan
- duration: 0.38s
- exit code: 2
FATAL Could not open defaultSettings.yamlI then applied the changes proposed above by @hamdanal to Then running gave me: which I think is what I would want to happen, given that the file in the repository had not previously been operated upon by I need to figure out why Perhaps we should be making use of the |
|
So it seems that things work. If you would run pre-commit once more you would find that it “passes” for latexindent. So indeed the EXE_FILES => ['latexindent.pl'],while it places the NAME => "LatexIndent",Then, use LatexIndentbut when looking for } elsif ( -e "$FindBin::RealBin/LatexIndent/defaultSettings.yaml" ) {looks in a subfolder of the binary directory, while in fact The fix by @hamdanal solves this my $dirName = dirname(__FILE__);points to the library directory, as that is where |
|
I'll take the liberty to implement all comments and fixes for a full and complete review |
tdegeus
changed the title
|
Ready for your review. As far as the implementation is concerned things are good to go I would say. However:
Thanks @hamdanal for the fix! |
|
Thanks, that's great. Thanks also for rebasing this from I've done some more testing, and using the following line in EXE_FILES => ['latexindent.pl','defaultSettings.yaml'],Can you check this at your end, and push a commit to your branch? Some more questions from me:
repos:
- repo: https://github.com/tdegeus/latexindent.pl
rev: 3.13.5
hooks:
- id: latexindent.pl
args: [-l]and was hoping that it would instruct
but how can I test how this will work when it's live on the repository?
I think we're getting very close to merging this! Thanks for your work :) |
|
Done I think :
|
|
@cmhughes I finally tested on some remote linux machine I got: so I'm not sure that your workaround actually works. Could you comment? |
|
Perhaps a better option is available from https://perldoc.perl.org/ExtUtils::MakeMaker Goal: copy defaultSettings.yaml to the LatexIndent folder. |
|
What I found from searching is exactly your solution. I just don't get why I didn't work for me. Maybe you had a slightly different syntax? Does the current commit work for you? |
|
Minor update: for me extra options work (tested using the |
|
The makefile seems to know to copy the LatexIndent folder without
explicitly being told to do so, probably because it knows to look for pm
files.
If we can find a way to make it do the same for yaml files, that might
help.
…On Wed, 19 Jan 2022, 11:47 Tom de Geus, ***@***.***> wrote:
Minor update: for me extra options work (tested using the
latexindent-conda hook)
—
Reply to this email directly, view it on GitHub
<#322 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQ7CYBJGM6ZWPVXXSGDOFLUW2QF7ANCNFSM5LTEZ3HA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
I tweaked NAME => "LatexIndent",Here is what I think happens:
This means that placing So now I think that that is what happens, and that that is solution; I'm not a
|
I agree. This would require changing any occurrence of
I think this should be avoided as it becomes easy to update one file and not the other. It would be an unnecessary added burden on the maintainer to always keep the two copies in sync. |
|
If defaultSettings.yaml is to be moved, please let me do it on develop.
There are a lot of consequences to this :)
I'm going to continue to try to avoid having to do this, updates to follow
when I have them.
…On Wed, 19 Jan 2022, 15:54 hamdanal, ***@***.***> wrote:
This means that placing defaultSettings.yaml in LatexIndent would solve
everything.
I agree. This would require changing any occurrence of
.../defaultSettings.yaml to .../LatexIndent/defaultSettings.yaml. I am
thinking in:
1. the builders (in github actions and elsewhere);
2. the LatexIndent/GetYamlSettings.pm file
...(or placing a copy there)
I think this should be avoided as it becomes easy to update one file and
not the other. It would be an unnecessary added burden on the maintainer to
always keep the two copies in sync.
—
Reply to this email directly, view it on GitHub
<#322 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAQ7CYCGWEZVTJLCKJTQNGTUW3NFHANCNFSM5LTEZ3HA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@cmhughes I completely understand. Let us, therefore, circle back to what you commented yesterday
So I thought: that's odd, why doesn't it work for me? In the documentation it says:
So what happened it seems is that I must say that relying on the first line not being a |
|
P.S. speaking of pre-commit: it provides great tools to keep source files clean of trailing spaces. Some editors, like mine, are set to remove them, leading to noise in PRs. (Sorry for staging the entire file ;)) |
|
My previous work around is feeling increasingly like it is not a solution. |
|
Sometimes life is surprisingly simple. Please checkout the latest commit !! |
|
That's great, nicely done. I think we're getting closer to merging this. basic testThe following test worked as expected: test with argumentsI wanted to try testing customisation of repos:
- repo: ../../projects/tdegues-latexindent
rev: 730d449e6506b59f12105a17184bfaa3c1e8034a
hooks:
- id: latexindent
args: [-l,-m]I could then run the following command successfully: I verified it by checking my questions
|
|
Great that you can confirm that it works. I let you decide how/if you CI on this.
|
…or `defaultSettings.yaml` for this case. Adding pre-commit hook (with documentation).
|
That should do it @cmhughes |
|
This is great, thank you! I'm really grateful for your contribution, and for your time in answering my questions. I've completed testing, and I'm very happy to merge this now. The I've put some details below, purely for my own reference, should I need to return to this. details for my referencesetting uptestingthis was successful testing argsthis seemed difficult with try-repo, so I followed the instructions at pre-commit/pre-commit#850 repos:
- repo: ../../projects/tdegues-latexindent
rev: 730d449e6506b59f12105a17184bfaa3c1e8034a
hooks:
- id: latexindent
args: [-l,-m]I could then run and verify, via |
|
This will be useful. Thank you both very much. |
cmhughes
added
pre-commit
Fixes #316
MakeFile.PLway, but the one I wrote does not download dependencies it seems. I kept my attempt for discussion.condawhere everything is working I think. However, I cannot test on my machine until Run without Unicode::GCString ? #303 is fixed. @cmhughes any news on this?