mohkale / projection

Projectile like project management library built on Emacs project.el
GNU General Public License v3.0
53 stars 2 forks source link

Using defcustom instead defvar for projections commands variables #8

Closed DevelopmentCool2449 closed 3 months ago

DevelopmentCool2449 commented 3 months ago

Using Projection command variables in .dir-locals cause showing those variables as unsafe, this can be fixed using a defcustom.

Also using defcustom makes them more easy to modify with recently customize-dirlocals in emacs 30.

mohkale commented 3 months ago

I don't think this'll really behave the way you expect to given the defcustom is generated by a macro and it won't be autoloaded. Add to that compile commands probably should be unsafe because you don't want a project to arbitrarily be able to set something that you then execute unwillingly. That said this should probably have been customizable to begin with.

DevelopmentCool2449 commented 3 months ago

I don't think this'll really behave the way you expect to given the defcustom is generated by a macro and it won't be autoloaded. Add to that compile commands probably should be unsafe because you don't want a project to arbitrarily be able to set something that you then execute unwillingly. That said this should probably have been customizable to begin with.

Using customize-dirlocals throw an error "Symbol’s function definition is void: nil" I've found it's a problem with ,cmd-var-symbol :type (optional string)

But i dont know which type to use in it.

mohkale commented 3 months ago

Could you share the actual function call you're running to produce that error?

DevelopmentCool2449 commented 3 months ago

Could you share the actual function call you're running to produce that error?

I have projection-commands-build-command to a string in my dir-locals, executing 'customize-dirlocals' in my project root says widget-match-inline: Symbol’s function definition is void: nil i found its a problem with projection-commands-build-command default value which is nil

mohkale commented 3 months ago

The default value has always been nil so I'm not sure why that would be problematic. Can you setup a demo project showing the problem or share a backtrace so I can see the full context of the issue.

DevelopmentCool2449 commented 3 months ago

dont worry i found that remplacing :type '(optional string) to :type '(sexp) fixs this. :type '(sexp) can be any lisp value so it can support strings and default nil value.

mohkale commented 3 months ago

That sounds like a bug in defcustom then. I'd suggest submitting a bug report to the Emacs mailing list. sexp isn't a good compromise since it lets you enter unsupported command values like '("foo" "bar") which'll trigger an error later on.

DevelopmentCool2449 commented 3 months ago

That sounds like a bug in defcustom then. I'd suggest submitting a bug report to the Emacs mailing list. sexp isn't a good compromise since it lets you enter unsupported command values like '("foo" "bar") which'll trigger an error later on.

It's a good point, what about using choice in :type something like :type '(choice (string :tag "String") (other :tag "Nothing")) instead?

i dont know which values projection-commands-*-command are planned to support.

mohkale commented 3 months ago

They only support string or unset (nil). If (choice string (const nil)) works I'd be ok switching to that but really this should be something fixed upstream.

DevelopmentCool2449 commented 3 months ago

They only support string or unset (nil). If (choice string (const nil)) works I'd be ok switching to that but really this should be something fixed upstream.

i dont think this can be a problem with defcustom due optional in :type (optional string) is not defined as a type, i didn't find it in the documentation, on the other hand my comment https://github.com/mohkale/projection/pull/8#issuecomment-1992498687 works even if command variable is default to nil and works well with strings.

mohkale commented 3 months ago

Ah, then we probably shouldn't be using it. Kinda annoying there's nothing linting the type field unless you actually use defcustom (which I don't :disappointed:).

DevelopmentCool2449 commented 3 months ago

Ah, then we probably shouldn't be using it. Kinda annoying there's nothing linting the type field unless you actually use defcustom (which I don't 😞).

I agree with you, the way defcustom works is stressful and managing the type field is complicated, but since customize-dirlocals landed to main branch i stopped editing by hand my dir-locals and using the new interface (which only use defcustoms), anyways i'm sastificed with just marking the variables as safe, this package is very useful for my daily work

mohkale commented 3 months ago

Hi @DevelopmentCool2449

Thought about it but I came to the conclusion you shouldn't be automatically running compile-commands set in dir-locals of whatever project you clone. I've kept the defcustom setup but changed the safety check. There's a section added to the README to disable the check the way you originally intended.

DevelopmentCool2449 commented 3 months ago

Hi @DevelopmentCool2449

Thought about it but I came to the conclusion you shouldn't be automatically running compile-commands set in dir-locals of whatever project you clone. I've kept the defcustom setup but changed the safety check. There's a section added to the README to disable the check the way you originally intended.

Thank you.