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

legit: add simple Mercurial support #1198

Merged
merged 2 commits into from
Dec 30, 2023
Merged

legit: add simple Mercurial support #1198

merged 2 commits into from
Dec 30, 2023

Conversation

vindarel
Copy link
Collaborator

This has been laying on a branch for some weeks, I think it's better put in use.

Support is simple, and it will be hard to have great support for different VCSs, but starting support for 3 of them helped me modularize my code and spot some missing cases in Git. So it's beneficial to have it IMO.

- see current changes, current branch, untracked files
- see latest commits
- commit
@@ -1,7 +1,8 @@
(defpackage :lem/legit
(:use :cl
:lem)
(:export :legit-status)
(:export :legit-status
:*prompt-for-commit-abort-p*)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way than exporting this variable and making it part of the API?
For example, how about using lem:config?
Also, I personally think that the p suffix in naming should only be used for functions or function parameters (e.g. read's recursive-p).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea to put user-facing variables into lem:config, but this system currently has no support for documentation strings, and it has no namespace system (unlike package:*variable*). Is lem:config really appropriate? It stores values for stuff that we changed with a global command, such as the color theme, the font size, isearch-previous-string.

@@ -388,7 +621,7 @@ M src/ext/porcelain.lisp
(defun %maybe-lem-home ()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that the lem package does not exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is actually possible: I develop for Lem both in Lem and in Emacs. And in Emacs, I don't load Lem because… it is not clear to me how to do it, if I ever knew. Quickloading Lem isn't enough, one has to add its dependencies, etc.

So it's necessary for me that the porcelain package doesn't depend on Lem. And I think it's a good separation of concerns.

Copy link
Member

@cxxxr cxxxr Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see why.
However, the dependency on lem is still there, just hidden and not statically obvious.
Ideally, porcelain.lisp should be loosely coupled to lem, but with this, wouldn't it be better to be able to resolve symbols statically with lem:lem-home?
That should be solved with just (ql:quickload :lem).

Or I guess you could define defgeneric in the package on this side and define that method in legit.lisp to keep loose coupling.

Copy link
Member

@cxxxr cxxxr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your comments are very thoughtful and well-written.

I focused my comments on the API with external parties, so please check it out if you like.

extensions/legit/porcelain.lisp Show resolved Hide resolved
@cxxxr
Copy link
Member

cxxxr commented Dec 30, 2023

I will merge this PR.

@cxxxr cxxxr merged commit f1abb33 into lem-project:main Dec 30, 2023
1 check passed
@vindarel
Copy link
Collaborator Author

and I'll handle your reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants