Re: [Outreachy] [Guix Data Service]: Identify the slow parts of process

[Canan Talayhan]

>From this I'm guessing the temp_package_metadata table has only one
>row. My understanding is that this table would normally have as many
>rows as packages in the revision of Guix being processed. It might not
>be possible to reproduce the slowness of the query without more rows.

I've inserted one row just as an example. As you've already said,
the temp_package_metadata table should have as many rows
as package_metadata. After populated the temp_package_metadata
with 500 rows of package_metadata, the query takes a long time
as we expected.

I'm using Flame Graph to visualize the slow paths on the revision part.
At first, I choose the slow one that I already know.
However, I can't successfully trigger the slow query following the below step:

* Run the **guix-data-service-process-job** under guix-data-service/scripts
folder as standalone providing an existing revision on my local db.

Am I on the right path for adding new jobs log to my local db?

In addition, I've successfully generated simple Flame Graph using Linux perf.
It visualizes only the data that was captured while I'm browsing on the
Guix Data Service Page. Please find the svg file attached.

Thanks,
Canan Talayhan

On Tue, Apr 27, 2021 at 9:26 PM Christopher Baines  wrote:
>
>
> Canan Talayhan  writes:
>
> > I am writing to give you an update on the progress that I have made.
>
> Great :)
>
> > I've created a temporary table named temp_package_metadata[1] and
> > insert a revision that already in my local database[2]. Then as you
> > said I've run the slow query with EXPLAIN ANALYZE. (screenshot is
> > attached) I may understand the slow query's working logic.
> >
> > [1]CREATE TEMPORARY TABLE temp_package_metadata (LIKE package_metadata
> > INCLUDING ALL)
> >
> > [2]INSERT INTO temp_package_metadata (home_page,
> > location_id,license_set_id,package_description_set_id,
> > package_synopsis_set_id) VALUES ('https://zlib.net/',9,9,2373,1407)
>
> From this I'm guessing the temp_package_metadata table has only one
> row. My understanding is that this table would normally have as many
> rows as packages in the revision of Guix being processed. It might not
> be possible to reproduce the slowness of the query without more rows.

[Outreachy] [Guix Data Service]: Identify the slow parts of process

[Canan Talayhan]

Hi Chris,

I am writing to give you an update on the progress that I have made.

I've created a temporary table named temp_package_metadata[1] and
insert a revision that already in my local database[2]. Then as you
said I've run the slow query with EXPLAIN ANALYZE. (screenshot is
attached) I may understand the slow query's working logic.

[1]CREATE TEMPORARY TABLE temp_package_metadata (LIKE package_metadata
INCLUDING ALL)

[2]INSERT INTO temp_package_metadata (home_page,
location_id,license_set_id,package_description_set_id,
package_synopsis_set_id) VALUES ('https://zlib.net/',9,9,2373,1407)

Now, I'm looking into a specific issue that arises when called
insert-missing-data-and-return-all-ids.

Thanks,
Canan Talayhan

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

[Canan Talayhan]

Thanks for your quick response. It helps a lot to me. But still, I
have some confusion about the reproduction steps. As I understand it,
I can reproduce the slow query just using the pure SQL queries without
touching the code for now, right?

Please find my steps below:

1. CREATE TEMPORARY TABLE temp_package_metadata (LIKE package_metadata
INCLUDING ALL)

As we expected, it creates a table but has no data. So I want to
insert some data but colon types are integer and I don't know any
meaningful data for these colons.

2. CREATE TEMPORARY TABLE temp_package_metadata AS TABLE package_metadata

Then I create a copy of the package_metadata.

3. EXPLAIN ANALYZE SELECT * FROM temp_package_metadata
This code generates 155 msec execution time, but I think it should
take more time.

Am I still on the right track?


On Sat, Apr 24, 2021 at 2:39 PM Christopher Baines  wrote:
>
>
> Christopher Baines  writes:
>
> > The approach I'd recommend is, make yourself a realistic
> > temp_package_metadata table by populating it with all the
> > package_metadata entries for a single revision already in your local
> > database. Then construct and try the slow query, and see how long it
> > takes, and look at the query plan (run the query with EXPLAIN at the
> > start).
>
> Following up on your question on IRC about creating the temporary table,
> the code that does this is here [1]. I wouldn't suggest running the
> code, just the same SQL it uses, and use relevant values for
> temp-table-name and table-name that are appropriate for the
> package_metadata table.
>
> 1: 
> https://git.savannah.gnu.org/cgit/guix/data-service.git/tree/guix-data-service/model/utils.scm#n313

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

[Canan Talayhan]

It seems after testing lots of pages this one escaped me since I only
tested the working case.

Please find the quick fix in the link below.
https://pastebin.ubuntu.com/p/s7tWyPHZ8F/

I'm looking forward to making another contribution. Could you please
review it as soon as possible?

Thanks,
Canan Talayhan


On Thu, Apr 22, 2021 at 10:47 PM Christopher Baines  wrote:
>
>
> Canan Talayhan  writes:
>
> > I've missed it unintentionally. I've not touched the "@" sign this time. :)
> >
> >>>  + (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 "
> >
> > I think I misunderstood that comment.
> >>> 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.
> >
> > Now, I've fixed it this way. I hope this version is good.
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > *(layout   #:title   (string-append "Comparing " (string-take base-commit
> > 8) " and "(string-take target-commit 8))   #:body   `(,(header)
> >  (div  (@ (class "container"))  (div   (@ (class "row"))
> >  (div(@ (class "col-sm-7")),@(if invalid-query?
> >   `((h1 "Compare"))  `((h1 "Comparing "(a
> > (@ (href ,(string-append "/revision/" base-commit)))
> >  (samp ,(string-take base-commit 8) "…"))" and "*
> >
>
> I think your email came through a bit garbled, anyway, I think there's
> still an issue with the title here.
>
> > On Mon, Apr 19, 2021 at 10:16 PM Christopher Baines 
> > wrote:
> >
> >>
> >> Canan Talayhan  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 
> >> wrote:
> >> >>
> >> >>
> >> >> Canan Talayhan  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
> >> >> >
> >> >&

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

[Canan Talayhan]

I've missed it unintentionally. I've not touched the "@" sign this time. :)

>>  + (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 "

I think I misunderstood that comment.
>> 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.

Now, I've fixed it this way. I hope this version is good.


















*(layout   #:title   (string-append "Comparing " (string-take base-commit
8) " and "(string-take target-commit 8))   #:body   `(,(header)
 (div  (@ (class "container"))  (div   (@ (class "row"))
 (div(@ (class "col-sm-7")),@(if invalid-query?
  `((h1 "Compare"))      `((h1 "Comparing "(a
(@ (href ,(string-append "/revision/" base-commit)))
 (samp ,(string-take base-commit 8) "…"))" and "*

I'm using VS Code, and sometimes it adds odd spaces to the code. Maybe I
need to switch to another code editor.

Thanks for all,
Canan Talayhan


On Mon, Apr 19, 2021 at 10:16 PM Christopher Baines 
wrote:

>
> Canan Talayhan  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 
> wrote:
> >>
> >>
> >> Canan Talayhan  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))
&

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

[Canan Talayhan]

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?

On Sun, Apr 18, 2021 at 8:53 PM Christopher Baines  wrote:
>
>
> Canan Talayhan  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.
>
>   @@ -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?
>
>   @@ -435,7 +444,7 @@
> " and "
> (a (@ (href ,(string-append "/revision/" 
> target-commit)))
>(samp ,(string-take target-commit 8) "…")))
>   -   '("Comparing derivations")
>   +`,page-header
>
> The quote then immediate unquote here isn't necessary, also, I think
> this should stick to being a list containing a string, as the other part
> of the if returns a list.
>
>   diff --git a/guix-data-service/web/dumps/html.scm 
> b/guix-data-service/web/dumps/html.scm
>   index 71e69c8..9645f7c 100644
>   --- a/guix-dat

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

[Canan Talayhan]

Hi Chris,

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/

Could you please review the patch attached?
I'm very excited to make my first FOSS contribution. :)

Thanks for your help,
Canan Talayhan


On Fri, Apr 16, 2021 at 2:11 PM Christopher Baines  wrote:

>
> Canan Talayhan  writes:
>
> > After your suggestions, I've fixed the patch. But for the revision
> > part, I have a question. You said "I'd put "Channel News Entries"
> > first, then the Revision bit second." There are so many titles like
> > this structure in Revision views.
> >
> > For example :
> > ->System tests Revision c7d0441
> > or
> > ->Revision c7d0441 System tests
> >
> > Should I fix all of them this way? Or just for Channel News Entries?
>
> I think having the System tests bit first is better, that way different
> pages about the same revision will have more distinct titles, and yeah,
> I'd apply the same principle to other titles as well.
>
From f3d02bf0e7050f544e044b8dd53d52e9dba28d54 Mon Sep 17 00:00:00 2001
From: Canan Talayhan 
Date: Sun, 18 Apr 2021 14:50:20 +0300
Subject: [PATCH] Set a more informative page title for any page where the
 title is "Guix Data Service"

---
 guix-data-service/web/build-server/html.scm | 24 +-
 guix-data-service/web/build/html.scm|  6 +-
 guix-data-service/web/compare/html.scm  | 32 ++--
 guix-data-service/web/dumps/html.scm|  7 +-
 guix-data-service/web/jobs/html.scm | 25 +-
 guix-data-service/web/nar/html.scm  |  6 +-
 guix-data-service/web/package/html.scm  |  6 +-
 guix-data-service/web/repository/html.scm   | 30 ++-
 guix-data-service/web/revision/html.scm | 89 +++--
 guix-data-service/web/view/html.scm | 12 ++-
 10 files changed, 209 insertions(+), 28 deletions(-)

diff --git a/guix-data-service/web/build-server/html.scm b/guix-data-service/web/build-server/html.scm
index f16a570..541a960 100644
--- a/guix-data-service/web/build-server/html.scm
+++ b/guix-data-service/web/build-server/html.scm
@@ -27,7 +27,11 @@
 (define (view-build query-parameters
 build
 required-failed-builds)
+  (define page-header "Build")
+
   (layout
+   #:title
+   page-header
#:body
`(,(header)
  (div
@@ -36,7 +40,7 @@
(@ (class "row"))
(div
 (@ (class "col-sm-12"))
-(h1 "Build")))
+(h1 ,page-header)))
   (div
(@ (class "row"))
,@(match build
@@ -98,7 +102,11 @@
 '())
 
 (define (view-build-servers build-servers)
+  (define page-header "Build servers")
+
   (layout
+   #:title
+   page-header
#:body
`(,(header)
  (div
@@ -107,7 +115,7 @@
(@ (class "row"))
(div
 (@ (class "col-sm-12"))
-(h2 "Build servers")
+(h2 ,page-header)
 ,@(map
(match-lambda
  ((id url lookup-all-derivations? lookup-builds?)
@@ -127,7 +135,11 @@
build-servers)))
 
 (define (view-build-server build-server)
+  (define page-header "Build server")
+
   (layout
+   #:title
+   page-header
#:body
`(,(header)
  (div
@@ -136,7 +148,7 @@
(@ (class "row"))
(div
 (@ (class "col-sm-12"))
-(h2 "Build server")
+(h2 ,page-header)
 ,(match build-server
((url lookup-all-derivations?)
 `(dl
@@ -150,7 +162,11 @@
"No")))
 
 (define (view-signing-key sexp)
+  (define page-header "Signing key")
+
   (layout
+   #:title
+   page-header
#:body
`(,(header)
  (div
@@ -159,5 +175,5 @@
(@ (class "row"))
(div
 (@ (class "col-sm-12"))
-(h2 "Signing key")
+(h2 ,page-header)
 ,(sexp-div sexp)))
diff --git a/guix-data-service/web/build/html.scm b/guix-data-service/web/build/html.scm
index 18d045a..4b758bb 100644
--- a/guix-data-service/web/build/html.scm
+++ b/guix-data-service/web/build/html.scm
@@ -29,7 +29,11 @@
  valid-targets
  stats
  builds)
+  (define page-he

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

[Canan Talayhan]

Hi again,

@Chris
After your suggestions, I've fixed the patch. But for the revision
part, I have a question. You said "I'd put "Channel News Entries"
first, then the Revision bit second." There are so many titles like
this structure in Revision views.

For example :
->System tests Revision c7d0441
or
->Revision c7d0441 System tests

Should I fix all of them this way? Or just for Channel News Entries?

I've set the rule for 80 chars. Thanks. :)

On Fri, Apr 16, 2021 at 12:52 AM Christopher Baines  wrote:
>
>
> Canan Talayhan  writes:
>
> > Hi team,
> >
> > Thanks for your help.
> >
> > @Chris
> > After all the modifications that I've made according to your comments,
> > I've created the latest version of my patch.
> > Could you please review the patch attached and share your ideas?
> >
> > Please note that a few parts are left. After your confirmation, I can
> > handle it shortly.
>
> Thanks Canan, I've had a quick look:
>
> > +(define page-header "Dumps")
> > +
> >  (define (view-dumps available-dumps)
> >(layout
> > +   #:title
> > +   page-header
> > #:body
> > `(,(header)
> >   (div
> > @@ -31,7 +35,7 @@
> > (@ (class "row"))
> > (div
> >  (@ (class "col-sm-12"))
> > -(h1 "Database dumps")))
> > +(h1 ,page-header)))
>
> I'd generally stick with more descriptive titles, "Database dumps" is
> far clearer than "Dumps" in this case.
>
> > +  (define page-header "Recent Events")
> > +
> >(layout
> > +   #:title
> > +   page-header
> > #:body
> > `(,(header)
> >   (div
> > @@ -200,7 +208,7 @@
> >  (@ (class "col-sm-12"))
> >  (a (@ (href "/jobs"))
> > (h3 "Jobs"))
> > -(h1 "Recent events")))
> > +(h1 ,page-header)))
>
> The capitalisation used here "events" rather than "Events" is
> intentional, I wouldn't change it while looking at the titles at least.
>
> >  (define (view-job job-id query-parameters log)
> > +  (define page-header "Job")
> > +
> >(layout
> > +   #:title
> > +   (string-append page-header " " job-id)
> > #:body
> > `(,(header)
> >   (div
> > @@ -339,7 +356,7 @@
> > (@ (class "row"))
> > (div
> >  (@ (class "col-sm-12"))
> > -(h1 "Job " ,job-id)))
> > +(h1 ,page-header ," " ,job-id)))
>
> I'd be tempted to combine the Job string and the job-id then use it for
> both the #:title and h1 element.
>
> > +   (string-append page-header " " (string-take commit-hash 7) " Channel 
> > News Entries")
>
> This title looks good, I'd put the more specific bits nearer the start
> though, so I'd put "Channel News Entries" first, then the Revision bit
> second. Also, watch out for the line length, I'd indent this so it's not
> longer than 80 lines.

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

[Canan Talayhan]

Hi team,

Thanks for your help.

@Chris
After all the modifications that I've made according to your comments,
I've created the latest version of my patch.
Could you please review the patch attached and share your ideas?

Please note that a few parts are left. After your confirmation, I can
handle it shortly.

Patch name: 0001-Set-a-more-informative-page-title-for-any-page


On Tue, Apr 13, 2021 at 8:51 PM Maxime Devos  wrote:
>
> On Tue, 2021-04-13 at 18:56 +0300, Canan Talayhan wrote:
> > [...]
> > After sending the patch I've turned the patch like below.
> >   (title ,(if title
> > `,(string-append title " - Guix Data Service")
> > '("Guix Data Service")))
>
> A little more simplification is possible:
>
> `,(string-append title ...) --> (string-append  ...)
>
> The code `,A after macro-expansion simply becomes A, for every A.
> To test, you can run
>   ,expand `,(string-append title " something")
> in a Guile REPL.
>
> Also,
>  '("Guix Data Service")))
> seems incorrect.  The end result if title is #f would be
>   (title ("Guix Data Service")),
> while I believe you wanted
>   (title "Guix Data Service").
>
> Correct code would be
>
> >  (title ,(if title
> > `,(string-append title " - Guix Data Service")
> > "Guix Data Service"))
> or alternatively
>
> >  (title ,(if title
> > `,(string-append title " - Guix Data Service")
> > '"Guix Data Service"))
> or
> >  (title ,(if title
> > `,(string-append title " - Guix Data Service")
> > `"Guix Data Service"))
>
> I don't know how familiar you are with quasiquote (`), quote (') and
> unquote (,); it may be worthwhile to look this up in the Guile maual
>
>   `,(string-append title " - Guix Data Service")
>
> I recommend sending e-mails as plain text.  One benefit is that plain
> text doesn't mess up code indentation (at least on e-mail clients using
> fixed-width fonts for plain text, which seems standard).
>
> Greetings,
> Maxime.
commit f1a85cdf2beecdc3d01b1242088182e9a3a2af11
Author: Canan Talayhan 
Date:   Thu Apr 8 17:29:37 2021 +0300

Set a more informative page title for any page
where the title is "Guix Data Service"
---
 guix-data-service/web/build/html.scm|  6 ++-
 guix-data-service/web/dumps/html.scm|  6 ++-
 guix-data-service/web/jobs/html.scm | 25 +--
 guix-data-service/web/nar/html.scm  |  6 ++-
 guix-data-service/web/package/html.scm  |  6 ++-
 guix-data-service/web/revision/html.scm | 73 +++--
 guix-data-service/web/view/html.scm | 12 --
 7 files changed, 119 insertions(+), 15 deletions(-)

diff --git a/guix-data-service/web/build/html.scm b/guix-data-service/web/build/html.scm
index 18d045a..4b758bb 100644
--- a/guix-data-service/web/build/html.scm
+++ b/guix-data-service/web/build/html.scm
@@ -29,7 +29,11 @@
  valid-targets
  stats
  builds)
+  (define page-header "Builds")
+
   (layout
+   #:title
+   page-header
#:body
`(,(header)
  (div
@@ -38,7 +42,7 @@
(@ (class "row"))
(div
 (@ (class "col-sm-12"))
-(h1 "Builds")
+(h1 ,page-header)
 (table
  (@ (class "table"))
  (thead
diff --git a/guix-data-service/web/dumps/html.scm b/guix-data-service/web/dumps/html.scm
index 71e69c8..c35cc1e 100644
--- a/guix-data-service/web/dumps/html.scm
+++ b/guix-data-service/web/dumps/html.scm
@@ -21,8 +21,12 @@
   #:use-module (guix-data-service web view html)
   #:export (view-dumps))
 
+(define page-header "Dumps")
+
 (define (view-dumps available-dumps)
   (layout
+   #:title
+   page-header
#:body
`(,(header)
  (div
@@ -31,7 +35,7 @@
(@ (class "row"))
(div
 (@ (class "col-sm-12"))
-(h1 "Database dumps")))
+(h1 ,page-header)))
   ,@(map
  (match-lambda
((date-string . files)
diff --git a/guix-data-service/web/jobs/html.scm b/guix-data-service/web/jobs/html.scm
index 82734d6..54f6aaf 100644
--- a/guix-data-service/web/jobs/html.scm
+++ b/guix-data-service/web/jobs/html.scm
@@ -30,7 +30,11 @@
jobs-and-events
recent-events
show-next-page?)
+  (define page-header "Jobs")
+
   (layout
+   #:title
+   page-header
#:body
`(,(header)
  (div
@@ -40,7 +44,7 @@
(div
 (@ (class "col-sm-12"))
 (h1 (@ (style "

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

[Canan Talayhan]

Hi Maxime,

Is this ‘introductory task’ publicly available?  If so, could you post a
> link?

I'm not up-to-date with Outreachy.
>

It's not available under the Official Outreachy page if you're not logged
in.


> Typographical nitpick: I would use a proper dash here (the figure dash ‒
> or the —
> em dash, I always forgot which is correct), instead of the hyphen-minus -.
> (Resource: <https://en.wikipedia.org/wiki/Dash#Figure_dash>).
>

It's a good catch : ) done.

You could bring the 'if' inside:
>   `((title ,(if title
> (string-append title " ‒ Guix Data Service")
> "Guix Data Service")))
>
>
> and a simplification is possible, as adding a title is unconditional:
>   (head
> (title ,(if title (string-append ...) ...))
> ...)
>

After sending the patch I've turned the patch like below.
  (title ,(if title
`,(string-append title " - Guix Data Service")
'("Guix Data Service")))

Nitpick: I would put a space before "Jobs"?  Also, what's the point of
> defining this variable, if it is constant?
>

We're generating titles using the available h1 value to prevent duplication.

Adding space between the id and job could be more readable, I agree. It's
done.

Thanks for the improvements,

Canan Talayhan

MSc. in Computer Engineering
Guix IRC: canant

On Tue, Apr 13, 2021 at 2:58 PM Maxime Devos  wrote:

> On Tue, 2021-04-13 at 12:01 +0300, Canan Talayhan wrote:
> > Hi everyone,
> Welcome!
>
> > My name is Canan. I'm an Outreachy applicant. I'm working on the
> introductory task for
> > Guix Data Service.
>
> Is this ‘introductory task’ publicly available?  If so, could you post a
> link?
> I'm not up-to-date with Outreachy.
>
> > Introductory task:
> > Set a more informative page title for any page where the title is "Guix
> Data Service"
> >
> > I've created a patch for the "Jobs" page. If it looks good for everyone
> then I can proceed with
> >  other applicable pages.
> >
> > Now, I'm working on the title part of the code snippet below to make it
> more elegant.
> >
> > ```scm
> > (define* (layout #:key
> >  (head '())
> >  (body '())
> >  title
> >  description)
> >   `((doctype "html")
> > (html
> >  (@ (lang "en"))
> >  (head
> >   ,@(if title
> > `((title ,(string-append title " - Guix Data Service")))
>
> Typographical nitpick: I would use a proper dash here (the figure dash ‒
> or the —
> em dash, I always forgot which is correct), instead of the hyphen-minus -.
> (Resource: <https://en.wikipedia.org/wiki/Dash#Figure_dash>).
>
> > `((title "Guix Data Service")))
> > ```
>
> You could bring the 'if' inside:
>   `((title ,(if title
> (string-append title " ‒ Guix Data Service")
> "Guix Data Service")))
>
>
> and a simplification is possible, as adding a title is unconditional:
>   (head
> (title ,(if title (string-append ...) ...))
> ...)
>
> > Could you please review and share your comments? I'll be appreciated.
> >
> > Attached file: jobs-title-and-view.diff
>
> +  (define page-header"Jobs")
>
> Nitpick: I would put a space before "Jobs"?  Also, what's the point of
> defining this variable, if it is constant?
>
> +  (define page-header "Job")
> +
>(layout
> +   #:title
> +   (string-append page-header job-id)
>
> IIRC, job ids are integers, so this would result in titles
> like "Job1234"?  Titles like "Job 1234" would be better.
>
> I haven't tested the patch, but better page titles are good, thanks!
>
> Maxime.
> Sometimes reviewing patches, but without actually testing them.
> Guix IRC: mdevos
>

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

[Canan Talayhan]

Hi everyone,

My name is Canan. I'm an Outreachy applicant. I'm working on the
introductory task for Guix Data Service.


Introductory task:

   - Set a more informative page title for any page where the title is
   "Guix Data Service"


I've created a patch for the "Jobs" page. If it looks good for everyone
then I can proceed with other applicable pages.


Now, I'm working on the title part of the code snippet below to make it
more elegant.


```scm

(define* (layout #:key

 (head '())

 (body '())

 title

 description)

  `((doctype "html")

(html

 (@ (lang "en"))

 (head

  ,@(if title

`((title ,(string-append title " - Guix Data Service")))

`((title "Guix Data Service")))

```


Could you please review and share your comments? I'll be appreciated.


Attached file: jobs-title-and-view.diff


Thanks,

Canan Talayhan

MSc. in Computer Engineering
Guix IRC: canant
 guix-data-service/web/jobs/html.scm | 25 +
 guix-data-service/web/view/html.scm | 11 +++
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/guix-data-service/web/jobs/html.scm b/guix-data-service/web/jobs/html.scm
index 82734d6..44d4b68 100644
--- a/guix-data-service/web/jobs/html.scm
+++ b/guix-data-service/web/jobs/html.scm
@@ -30,7 +30,11 @@
jobs-and-events
recent-events
show-next-page?)
+  (define page-header"Jobs")
+
   (layout
+   #:title
+   page-header
#:body
`(,(header)
  (div
@@ -40,7 +44,7 @@
(div
 (@ (class "col-sm-12"))
 (h1 (@ (style "display: inline-block;"))
-"Jobs")
+,page-header)
 (div
  (@ (class "btn-group pull-right")
 (style "margin-top: 1.3rem;")
@@ -189,7 +193,11 @@
 
 (define (view-job-events query-parameters
  recent-events)
+  (define page-header "Recent Events")
+
   (layout
+   #:title
+   page-header
#:body
`(,(header)
  (div
@@ -200,7 +208,7 @@
 (@ (class "col-sm-12"))
 (a (@ (href "/jobs"))
(h3 "Jobs"))
-(h1 "Recent events")))
+(h1 ,page-header)))
   (div
(@ (class "row"))
(div
@@ -256,7 +264,11 @@
  recent-events)
 
 (define (view-job-queue jobs-and-events)
+  (define page-header "Queued Jobs")
+
   (layout
+   #:title
+   page-header
#:body
`(,(header)
  (div
@@ -267,7 +279,7 @@
 (@ (class "col-sm-12"))
 (a (@ (href "/jobs"))
(h3 "Jobs"))
-(h1 "Queued jobs ("
+(h1 ,page-header"("
 ,(length jobs-and-events)
 ")")))
   (div
@@ -329,8 +341,13 @@
  '())
  jobs-and-events)
 
+
 (define (view-job job-id query-parameters log)
+  (define page-header "Job")
+
   (layout
+   #:title
+   (string-append page-header job-id)
#:body
`(,(header)
  (div
@@ -339,7 +356,7 @@
(@ (class "row"))
(div
 (@ (class "col-sm-12"))
-(h1 "Job " ,job-id)))
+(h1 ,page-header ,job-id)))
   (div
(@ (class "row"))
(div
diff --git a/guix-data-service/web/view/html.scm b/guix-data-service/web/view/html.scm
index 8063e17..94ea9a1 100644
--- a/guix-data-service/web/view/html.scm
+++ b/guix-data-service/web/view/html.scm
@@ -65,13 +65,15 @@
 (define* (layout #:key
  (head '())
  (body '())
- (title "Guix Data Service")
+ title
  description)
   `((doctype "html")
 (html
  (@ (lang "en"))
  (head
-  (title ,title)
+  ,@(if title
+`((title ,(string-append title " - Guix Data Service")))
+`((title "Guix Data Service")))
   (meta (@ (http-equiv "Content-Type")
(content "text/html; charset=UTF-8")))
   (meta (@ (name "viewport")
@@ -286,8 +288,7 @@
 (define (index git-repositories-and-revisions)
   (layout
#:description
-   "The Guix Data Service processes, stores and provides data about Guix over
-time."
+   "The Guix Data Service processes, stores and provides data about Guix over time."
#:body
`(,(header)
  (div
@@ -335,6 +336,8 @@ time."
 
 (define (view-statistics guix-revisions-count derivations-count)
   (layout
+   #:title
+   "Statistics"
#:body
`(,(header)
  (div