Re: [Outreachy] - Guix Data Service - Set a more informative page title

[Christopher Baines <mail@cbaines.net>]

[-- Attachment #1: Type: text/plain, Size: 5775 bytes --]


Canan Talayhan <canan.t.talayhan@gmail.com> writes:

> Thanks for your quick response.
>
>>Why's the @ being removed here?
> It interprets like an HTML code when I use the page-header like
> `,page-header, so I removed it. According to your comment, I reverted
> to the original version.
>
> " 'GET repository..." which includes package/package-name in the URL
> has not the best titles since I couldn't test them because of the
> error that I've mentioned.
> I'm open to suggestions.
>
> Could you please re-review the patch that contains all the
> modifications you've mentioned in the previous message?

I've had another look, see my comments below.

> On Sun, Apr 18, 2021 at 8:53 PM Christopher Baines <mail@cbaines.net> wrote:
>>
>>
>> Canan Talayhan <canan.t.talayhan@gmail.com> writes:
>>
>> > I've updated the patch that contains all the suggestions. I think the patch
>> > is ready to merge.
>> >
>> > One thing that I would like to ask you about the package and package-name
>> > in web/repository/controller.scm.
>> >
>> > When I test the URL below I'm getting this error. (
>> > https://pastebin.ubuntu.com/p/HdKShmKqH7/)
>> >
>> >    - ('GET "repository" repository-id "branch" branch-name "package"
>> >    package-name) ->
>> >    http://localhost:8765/repository/1/branch/master/package/emacs
>> >
>> > What do you think? BTW it's accessible on the official server.
>> >
>> >    - https://data.guix.gnu.org/repository/1/branch/master/package/emacs/
>>
>> Hmm, this could possibly be due to an issue with the small dump of the
>> database.
>>
>> > Could you please review the patch attached?
>> > I'm very excited to make my first FOSS contribution. :)
>>
>> I've had a look though, and I have some more comments:
>>
>>   diff --git a/guix-data-service/web/compare/html.scm b/guix-data-service/web/compare/html.scm
>>   index 5b5fe0a..170fb12 100644
>>   --- a/guix-data-service/web/compare/html.scm
>>   +++ b/guix-data-service/web/compare/html.scm
>>   @@ -96,7 +96,12 @@
>>        (unless invalid-query?
>>          (query-parameters->string query-parameters)))
>>
>>   +  (define page-header "Comparing")
>>   +
>>      (layout
>>   +   #:title
>>   +   (string-append page-header " " (string-take base-commit 8) " and "
>>   +    (string-take target-commit 8))
>>       #:body
>>       `(,(header)
>>         (div
>>   @@ -107,7 +112,7 @@
>>            (@ (class "col-sm-7"))
>>            ,@(if invalid-query?
>>                  `((h1 "Compare"))
>>   -              `((h1 "Comparing "
>>   +              `((h1 ,page-header ," "
>>                        (a (@ (href ,(string-append "/revision/" base-commit)))
>>                           (samp ,(string-take base-commit 8) "…"))
>>                        " and "
>>
>> There's a couple of things here. I'd be tempted not to use a variable
>> for "Comparing", it's not really the page header, as that's more
>> complicated, so I think I'd just use the string in both places.
>>
>> Second thing, the (if invalid-query? bit when constructing the h1
>> element is important. The query parameters being invalid could mean
>> anything from the form just hasn't been filled in, to the value isn't
>> actually a commit hash, but something else, maybe some HTML/JavaScript
>> that is malicious and shouldn't be included in the page. A similar
>> approach probably needs taking for the title.

This stuff above still looks the same to me, although part of my
analysis was wrong, as the type of an invalid parameter is a record, so
the page just breaks if the parameters are invalid (which I guses is
better than what I was describing).

Anyway, I think this still needs fixing.

>>   @@ -419,14 +424,18 @@
>>        '(span (@ (class "text-success glyphicon glyphicon-plus pull-left")
>>                  (style "font-size: 1.5em; padding-right: 0.4em;"))))
>>
>>   +  (define page-header "Comparing derivations")
>>   +
>>      (layout
>>   +   #:title
>>   +   page-header
>>       #:body
>>       `(,(header)
>>         (div
>>          (@ (class "container"))
>>          (div
>>           (@ (class "row"))
>>   -       (h1 ,@(let ((base-commit (assq-ref query-parameters 'base_commit))
>>   +       (h1 ,(let ((base-commit (assq-ref query-parameters  'base_commit))
>>
>> Why's the @ being removed here?

This is still being removed.

  @@ -435,7 +439,7 @@
                        " and "
                        (a (@ (href ,(string-append "/revision/" target-commit)))
                           (samp ,(string-take target-commit 8) "…")))
  -                   '("Comparing derivations")))))
  +                    '("Comparing derivations")))))
         (div
          (@ (class "row"))
          (div

Watch out for unwanted indentation changes you're making, I think the
previous indentation was better.

  @@ -256,7 +264,12 @@
                recent-events)))))))))

   (define (view-job-queue jobs-and-events)
  +  (define page-header (string-append "Queued jobs ("
  +    (number->string (length jobs-and-events)) ")"))
  +
     (layout
  +   #:title
  +   page-header
      #:body
      `(,(header)
        (div

For the page-header here, I'd indent the code like so:

  (define page-header
    (string-append "Queued jobs ("
                   (number->string (length jobs-and-events))
                   ")"))

The main change being moving the code off the (define ... line. If it's
a one line thing that can fit, I think it's fine to have as one line,
but otherwise, I think this makes it easier to read.

The other thing to fix before merging is the commit message. The current
one is a bit odd, and the first line is too long.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]