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

add STYLEGUIDE #1443

Merged
merged 3 commits into from
Oct 3, 2024
Merged

add STYLEGUIDE #1443

merged 3 commits into from
Oct 3, 2024

Conversation

vindarel
Copy link
Collaborator

A styleguide can be beneficial to… guide new contributors.

There aren't a lot of "don't"s or "must"s.

I gathered remarks that were applied on some pull requests, and you might recognize one or two preferences of mine.

This first attempt is here for debate.

@vindarel vindarel requested a review from cxxxr July 16, 2024 18:36
@cxxxr
Copy link
Member

cxxxr commented Jul 17, 2024

I guess this is a letter for my usual messy changes.


## Loop

Loop keywords are written as keywords, with the `:`:
Copy link
Member

Choose a reason for hiding this comment

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

👍

Comment on lines +69 to +71
Don't write long macros, use the "call-with-" pattern.

Don't define lists with backquote and comma, use the `list` constructor.
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

Choose a reason for hiding this comment

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

it would be even better if there were some actual examples.

STYLEGUIDE.md Outdated Show resolved Hide resolved
STYLEGUIDE.md Show resolved Hide resolved

## Git and pull requests

Please rebase and squash small commits together (you can do this with lem/legit ! ;) ).
Copy link
Member

@cxxxr cxxxr Jul 20, 2024

Choose a reason for hiding this comment

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

What granularity are you assuming?
For example, is extracting functions, rename symbol, and changing to a call-with pattern too small to be included in a single commit?

@anlsh
Copy link
Contributor

anlsh commented Jul 22, 2024

Can we add a section on dynamic binding?

What I'm really asking: can we ban dynamic binding unless it's really necessary?

defvar/defparameter make a lot of sense for, say, defining variables which are to be customized by the user. But actually using dynamic extent to substitute for passing function parameters, a la

(defvar *var* 1)
(defun foo ()
   *var*)
(let ((*var* 2))
    (foo))

is, IMO, a trap. Sure, you save a function parameter: on the other hand, you torpedo your ability to run code in separate threads.

Obviously, it's not like avoiding dynamic binding will magically make all of our code thread safe, and Lem probably doesn't really use concurrency at the moment. But I think we should plan for the future and learn from Emacs's experience in this regard

@cxxxr
Copy link
Member

cxxxr commented Jul 22, 2024

I too think you are right to avoid unnecessary dynamic bindings as a good code writing practice.

However, dynamic bindings are thread-safe.
https://www.sbcl.org/manual/#Special-Variables

@anlsh
Copy link
Contributor

anlsh commented Jul 22, 2024

Oh wow, I had no idea that bindings were thread-local. Thanks for clearing that up

@cxxxr
Copy link
Member

cxxxr commented Oct 3, 2024

@vindarel Is it okay to merge this?

@vindarel
Copy link
Collaborator Author

vindarel commented Oct 3, 2024

Hi yes it is IMO, I edited it after your remarks.

@cxxxr cxxxr merged commit 23640ee into lem-project:main Oct 3, 2024
2 checks passed
@vindarel vindarel deleted the vindarel/add-STYLEGUIDE branch October 3, 2024 19:39
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.

3 participants