On Sat, Apr 6, 2024 at 5:35 AM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: Elijah G <eg642616@gmail.com>
> > Date: Wed, 27 Mar 2024 15:29:28 -0600
> > Cc: Eli Zaretskii <eliz@gnu.org>, Spencer Baugh <sbaugh@janestreet.com>, philipk@posteo.net,
> >       emacs-devel@gnu.org, Po Lu <luangruo@yahoo.com>
> >
> > I've now updated the patch to allow auto resize margins and using margins
> > when it's in terminal frames.
>
> Thanks, I think this is almost ready to be installed; see a few minor
> comments below.
>
> > Sadly I couldn't find anything for detecting HDiPI frames, so the user
> > must have to configure it.
>
> I don't think Emacs should second-guess the user in this matter: let
> the user decide whether the fringes are too narrow for these
> indicators.
>
> > +(defcustom flymake-indicator-type (if (display-graphic-p)
> > +                                      'fringes
> > +                                    'margins)
> > +  "Indicate which indicator type to use for display errors.
> > +
> > +The value can be nil (dont indicate errors but just highlight them),
>                          ^^^^
> "don't"
>
> > +(defcustom flymake-margin-indicators-string
> > +  '((error "!!" compilation-error)
> > +    (warning "!" compilation-warning)
> > +    (note "!" compilation-info))
> > +  "Strings used for margins indicators.
> > +The value of each list may be a list of 3 elements where specifies the
> > +error type, its string to use and its face,
>                ^^^^^^^^^^
> "the string"
>
> > +or a list of 2 elements specifying only the error type and its string.
>                                                           ^^^^^^^^^^^^^^
> "and the corresponding string"
>
> > +Indicators can be an ASCII or non-ASCII string.
>
> This sentence is redundant, so I suggest to remove it.  The ASCII vs
> non-ASCII issue is only relevant for the default value; users can do
> whatever they want when they customize the option.
>
> > +(defcustom flymake-autoresize-margins t
> > +  "If non-nil automatically resize margin-width.
>                 ^
> Comma is missing there.  Also, how about telling what triggers
> the resizing?
>
> > +Only if `flymake-indicator-type' is set to margins."
>    ^^^^^^^
> "Only relevant if..."
>
> > +(defvar-local flymake-original-margin-width nil
> > +  "Store original margin width")
>
> Should this be an internal variable (named flymake--original-margin-width)?
> Also, please consider telling in the doc string when is the width restored.
>
> > +(defun flymake--resize-margins (&optional og-width)
> > +  "Resize current window margins according `flymake-margin-indicator-position'.
>                                     ^^^^^^^^^
> "according to"
>
> > +Return to original margin width if OG-WIDTH is non-nil."
>
> Suggest to rename OG-WIDTH to ORIG-WIDTH.
>
> Last, but not least: please mention this change (and the new user
> options) in NEWS and in the Flymake manual.

Thank you, I've now fixed the patch and documented the changes done.

Also I have a question, Do I need to assign copyright each time
that I want to contribute to Flymake?

I have a lot of ideas that I would like to contribute.