Coder Social home page Coder Social logo

Comments (9)

Apemb avatar Apemb commented on June 19, 2024 1

So I tested your new improvements, and it all works great 🎉 ! (tested with {:cmd, "elixir ./priv/scripts/some_script.exs", include_hook_args?: true}, {:cmd, "mix some_task", include_hook_args?: true}, {:file, "./priv/scripts/some_script", include_hook_args?: true} and {:cmd, "mix format"}. All work as expected 👍 )

I have a small suggestion. When the commands run for exemple this one {:cmd, "mix format"}
The verbose output is

↗ Running hooks for :pre_commit
✔ `mix` was successful

Which is not as helpful as having the whole command written (mix format in that case).
I would make the output contain the complete command. What do you think ?

Edit :
In the documentation, I think the fact that "some command" and {:cmd, "some command"} are equivalent and do the same thing is not really clear, and might need a bit more explanations :)

from elixir_git_hooks.

qgadrian avatar qgadrian commented on June 19, 2024 1

Wonderful :)

Thanks for the other feedback btw, I'll fix it by the weekend and release the new version. I'll open new tickets for them and close this one.

Thanks again @Apemb

from elixir_git_hooks.

qgadrian avatar qgadrian commented on June 19, 2024

Hey @Apemb!

Yeah definitely this project needs to support the commit-msg hook properly.

Let me know if you want to work on a PR, otherwise I will try to find some time this week to fix the open issues :)

I just update the project so you can use {:file, "path_to_script"} and you will receive the git arguments as usual.

Under priv/test_script you will have a small example that will echo all the args for the configured hook (you can see in the README the config format).

I published a preview version 0.3.2-pre that I will try to validate on my projects, feel free to take a look at give your input.

Thanks for the contribution!

from elixir_git_hooks.

Apemb avatar Apemb commented on June 19, 2024

You Sir are awesome ! I upgraded my version and all seems to work perfectly with {:file, "path_to_script"}. I arrived this morning thinking to work on a nice PR and you already made it possible :-).

If possible I would suggest to make the mix tasks receive the arguments too. Creating a mix task is quite easy, easier than create a bash script for me at least.
This might allow also to not have to explicitly ask for the args like this {:file, "path_to_script"}. If the script is executable and the args are transferred any way path_to_script should be enough no ? (for now it launches the script but the arguments do not seem to be transferred)
Is there a technical hurdle to do it this way ? Something I do not see ?

Anyway thanks again, if I can help you in anyway please tell me

from elixir_git_hooks.

qgadrian avatar qgadrian commented on June 19, 2024

Sure, what about adding a {:mix_task, "task_name"} that will invoke the task with the hook arguments (if there are)?

By default, having a list of strings will run the plain commands, so we need to differentiate when we can send the hook arguments and when we cannot.

This will be really easy to implement, I'll have a new version by the end of today so you can give it a try :)

from elixir_git_hooks.

Apemb avatar Apemb commented on June 19, 2024

So juste for me to understand, that would mean three modes :

  • arbitrary command without arguments
  • arbitrary mix task with arguments
  • file path with arguments

Wouldn't it be better to have something like {:cmd_with_args, "mix task_name"} instead of {:mix_task, "task_name"} ? (since the default string command argument is reacting like {:cmd_without_args, "bash command"}
{:cmd_with_args, "bash command"} is the command with argument added to the command after bash command arg1 arg2 ?
Because probably {:mix_task, "task_name"} should react as the default arbitrary command to execute non ? Like {:cmd_with_args, "mix task_name"} ?

That would mean :

  • arbitrary command without arguments: "mix task_name"
  • arbitrary command with arguments appended after: {:cmd_with_args, "mix task_name"}
  • file path with arguments: {:file, "path_to_script"}

What do you think ? It is not a big change, I am not entirely sure it is worth the time. It was more to see if the interface could be standardized between the three modes, all of them being a tuple, with syntactic sugar for the default {:cmd_without_args, "mix task_name"} where one could only write a string.

The possibility to use a mix task is anyways very nice :) With additional modules like git_cli the task can do some nice things while being tested properly and not having to launch chmod +x on the script.

(for example right now my script file contains one line:

elixir ./priv/scripts/prepend-commit-message.exs $@

which could be put directly into a {:cmd_with_args, "elixir ./priv/scripts/prepend-commit-message.exs "} if the arguments are prepended after that command string)

from elixir_git_hooks.

qgadrian avatar qgadrian commented on June 19, 2024

I think it's better to standarize the way the actions are configured. This is, having cmd_with_args, cmd_without_args and file (although maybe we should have with and without args as well...).

I liked the cleaning less of just declaring a list of strings, but clearly it's not flexible enough.

What about having the git hook args filtered via opts? It would look something like this:

{:cmd, "elixir ./priv/scripts/prepend-commit-message.exs", include_hook_args?: true}

from elixir_git_hooks.

Apemb avatar Apemb commented on June 19, 2024

{:cmd, "elixir ./priv/scripts/prepend-commit-message.exs"} and {:file, "./priv/scripts/some_script"} seems like good abstractions. It seems clear to me :)
The include_hook_args?: true as an optional argument that is false by default and that one can put as true if needed seems nice and understandable too !

As of using only strings as command, allowing just strings that ends up being {:cmd, "command", include_hook_args?: false} would be fine IMHO, and has the advantage of making those new features not breaking change 👌

from elixir_git_hooks.

qgadrian avatar qgadrian commented on June 19, 2024

Hey @Apemb I just updated the project with the {:cmd, _} option as well. It includes the option include_hook_args to forward them to the command, [see README for examples(https://github.com/qgadrian/elixir_git_hooks#type-of-tasks).

There is another pre-release on hex named 0.3.2-pre3, let me know if it's working fine for you :)

from elixir_git_hooks.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.