guile-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] Added srfi-214: flexvectors


From: Vijay Marupudi
Subject: Re: [PATCH] Added srfi-214: flexvectors
Date: Sun, 13 Nov 2022 15:27:22 -0500

Hello,

I've made all the suggested edits. Sorry about the delay, universities
get quite busy in the middle of the semester.

> Please use the ChangeLog style for commit messages (see ‘git log’ for
> examples); as a welcome gift, we can do it for you though.  :-)

I've tried to do this, please verify this!
>
>   • please leave two spaces after end-of-sentence periods (a convention
>     eases navigation with Emacs);
>
>   • use @dfn to decorate a term the first time it’s introduced, @code
>     for identifiers, etc.;
>
>   • limit lines to ~80 columns.

Changed.


> Maybe add a first sentence before this one, like: “The @code{(srfi
> srfi-214)} module implements @dfn{flexvectors}, a data structure for
> mutable vectors with adjustable size.”

Added.

> You can drop @bullet{}.

Removed.

> Add a semicolon at the end of the first two lines, a period for the last
> one.  @code{ArrayList}.

Added.

> Nitpick: should be:
>
>   Copyright (C) 2022 …
>

Changed.

> Please use a single ‘define-module’ clause with #:use-module and
> #:export lines (this is equivalent but that’s what we conventionally
> use and it makes the interface clearer upfront).
>
> Avoid using R6RS and R7RS modules as they pull in a whole lot of stuff.
> For example, maybe you can use (ice-9 binary-ports) instead of (rnrs io
> ports); maybe you don’t need (scheme …) at all because Guile most likely
> has equivalent things built in.

Changed both to fit the convention.

> Don’t export <flexvector> as it would expose implementation details (the
> record interface).

I removed that export.

> In general we try to add docstrings for all public top-level
> procedures.  Bonus points if you can do that.  :-)

Ah, this would take way too long for me, so I'll leave this up to
another contributor in the future. Would be a great first contribution!

> The rest looks great to me!
>
> Could you send an updated patch?

I've attached the updated patch.

~ Vijay

Attachment: 0001-Added-srfi-214-flexvectors.patch
Description: Text Data


reply via email to

[Prev in Thread] Current Thread [Next in Thread]