Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(whaler): added whaler.el recipe #8894

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

SalOrak
Copy link

@SalOrak SalOrak commented Jan 23, 2024

Brief summary of what the package does

whaler.el allows moving between directories blazingly fast while providing a sense of current working directory

Direct link to the package repository

https://github.com/salorak/whaler.el

Your association with the package

Author and Maintainer

Relevant communications with the upstream package maintainer

None needed

Checklist

@riscy
Copy link
Member

riscy commented Jan 28, 2024

Thanks for this. Quick first pass --

whaler.el with byte-compile using Emacs 29.1:

whaler.el:41:2: Error: Cannot open load file: No such file or directory, f

I think a Package-Requires is needed on f in this case.

whaler.el with melpazoid:

- whaler.el#L3: Have you done the paperwork to assign this copyright?
- whaler.el#L12: This may be a copy-paste error?

I always check these, but line 12 seems to definitely not be the case (yet?)

@SalOrak
Copy link
Author

SalOrak commented Jan 28, 2024

Hii! Thank you so much for the time put and effort, I really appreciate it.

I've added melpazoid and run a couple of times using GA. It should be all fixed by now!

Thaaank youu!

@riscy
Copy link
Member

riscy commented Feb 4, 2024

Thank you for addressing those. I think some more has changed in the meantime. Here's the latest --

whaler.el with byte-compile using Emacs 29.1:

whaler.el:108:2: Warning: docstring has wrong usage of unescaped single quotes (use \= or different quoting)
In whaler--generate-subdirectories:
whaler.el:124:9: Warning: Unused lexical variable `value'
whaler.el:126:13: Warning: Unused lexical variable `el'

@SalOrak
Copy link
Author

SalOrak commented Feb 5, 2024

Okey! Thanks I thought I had it, daamn.

Now yes, this should be it. I updated the repo and should pass.

PS: In the GA I always get

  • No .el file matches the name 'shx'
  • Can't check Package-Requires if there is no 'main' file`.

Guess it does not apply here right?

If you need anything let me know, and again thank you for your time and effort.

@riscy
Copy link
Member

riscy commented Feb 12, 2024

No .el file matches the name 'shx'

My best guess is that the GitHub actions manifest needs to be updated with the name of your package. :)

But that puts the checklist (the easy part) behind us.

The biggest blocker I'm seeing is that a lot of this functionality (as far as I can tell) already exists in Emacs 29. For instance project-switch-project looks like it does what your current whaler-whaler function does, and project-find-file looks like it does what whaler-find-files-in-current-working-directory does, with more conventions for defining projects. Are you familiar with project.el?

Beyond that, a couple more small comments --

  • You define a function called whaler-whaler but of course whaler is also in your namespace - you should feel free to use that function name
  • whaler--execute-function-on-current-working-directory is private (whaler--) but interactive - I think the interactive tag is incorrect here?
  • You're calling ivy-read directly, but would a regular old completing-read work here? This would let users make use of their own completion frameworks instead of imposing one on them

@SalOrak
Copy link
Author

SalOrak commented Feb 24, 2024

Hi again! Thanks for the recommendation. I have updated the package accordingly.

Yes I'm aware of project. The thing is that whaler has a very small footprint and it is easily extended. I like knowing which repositories are in my list of directories, having full control of them. Whaler, as you can see, does not anything special but allows for a more tailored experience when switching directories.

@SalOrak
Copy link
Author

SalOrak commented Feb 25, 2024

After your comment about why Whaler.el should be include I've been thinking. Whaler offers an API to work with projects (directories, repositories) easily. The idea is that users have two main functions whaler and whaler-execute-function-on-current-working-directory and customize their experiences around projects.
For example:

  • Being able to search strings in another project using their tool/function of use without changing the current selected project.
  • Execute compile on the selected project ( or another).
    And so an and so forth.

I hope this makes more sense in the Emacs package community. If anything don't hesitate to contact me.

Also, any advice or possible ideas on the code are highly appreciate it. I'm still learning emacs but as you may know, it a rabbit hole of information.

And, of course, thank you for your time. I really appreciate all the comment and work done here :)

@riscy
Copy link
Member

riscy commented Feb 28, 2024

Okay - I think your argument is good, but then I'd probably ask that you update the ;;; Commentary with these kinds of examples. What you currently have may mislead:

;; It is a minimalistic project manager aiming to help move between
;; directories and find files as fast as possible.

(You mentioned compilation, for example).

Also your summary line is Move between directories blazingly fast which doesn't have "project" as a keyword, which I think is important.

I also feel (and correct me if I'm wrong) that there may have been some big shifts in the code since the last review. This might be a sign that you want to wait before indexing on MELPA for a couple weeks or a month until things have settled down.

Another small thing while I'm doing this pass - checkdoc has rightly told you to capitalize arguments, but that's all you do. For instance:

  "Generic function to execute in the current working directory.

The `ACTION' parameter represent the function to execute.

The `ACTION-ARG' parameter determines whether the current working directory
should be passed as an argument to the `ACTION' function.
By default is `t`.

should be

  "Generic function to execute in the current working directory.

The ACTION parameter represent the function to execute.

The ACTION-ARG parameter determines whether the current working directory
should be passed as an argument to the ACTION function.
By default is `t'.

@riscy
Copy link
Member

riscy commented Mar 10, 2024

Friendly ping. :)

@SalOrak
Copy link
Author

SalOrak commented Mar 10, 2024

Hi! Sorry, kept posponing the answer.

But yeah, I totally agree with you. The codebase has suffered a huge shift since I postes this. I think the best thing will be to qait for a couple more weeks and see if it is stable.

Moreover, I've updated the README (even though I'm still not convinced with the summary) and the docs of the code.

Again, thank you for your work, the effort and your time. I really appreciate it.

So in approximately in 2 weeks, around the end of March I'll comment again with the status of the project as well as if I consider it finished so we can check everything again.

@riscy
Copy link
Member

riscy commented Mar 17, 2024

Sounds good - I'll mark this awaiting-upstream in the meantime.

@riscy riscy added the awaiting-upstream Awaiting action from an upstream maintainer label Mar 17, 2024
@SalOrak
Copy link
Author

SalOrak commented Apr 18, 2024

Hello! Reborn from the ashes of time.

Sooo... I've been messing around with Whaler and I'm doing rewrite some parts of it to be more accessible and easy to use.

As you previously suggested, I want Whaler to be a really simple and minimalistic project manager. But at the same time, there are 'goodies' that comes from experiencing not only Whaler but Emacs (still learning, almost 6 month into it).

In conclusion, I'm still developing Whaler.el. I'll update this once it is ready.

Thank you so much for your time and effort.

Hope everything is going well!

@riscy
Copy link
Member

riscy commented Apr 21, 2024

Thank you for the update! I'll leave this in the awaiting-upstream state for now, but I appreciate that. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-upstream Awaiting action from an upstream maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants