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

[Christopher Baines]

Canan Talayhan  writes:

> 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.

So, the question to ask here is what data would the temporary table
usually contain during loading data for a revision (when the slow query
happens). I gave more specific information about the data in the
temporary table in my previous email.

> 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.

I don't see why that query should be slower, but that's also not that
relevant since I think the actual slow query in question here is quite
different.

You can find the query in the code by looking at where it does the
timing for "querying the temp_package_metadata".

I've also got information from the PostgreSQL logs, which gives this as
the query:

  SELECT package_metadata.id, package_metadata.home_page, 
package_metadata.location_id, package_metadata.license_set_id, 
package_metadata.package_description_set_id, 
package_metadata.package_synopsis_set_id FROM package_metadata INNER JOIN 
temp_package_metadata ON (package_metadata.home_page = 
temp_package_metadata.home_page OR (package_metadata.home_page IS NULL AND 
temp_package_metadata.home_page IS NULL)) AND (package_metadata.location_id = 
temp_package_metadata.location_id OR (package_metadata.location_id IS NULL AND 
temp_package_metadata.location_id IS NULL)) AND 
(package_metadata.license_set_id = temp_package_metadata.license_set_id OR 
(package_metadata.license_set_id IS NULL AND 
temp_package_metadata.license_set_id IS NULL)) AND 
(package_metadata.package_description_set_id = 
temp_package_metadata.package_description_set_id OR 
(package_metadata.package_description_set_id IS NULL AND 
temp_package_metadata.package_description_set_id IS NULL)) AND 
(package_metadata.package_synopsis_set_id = 
temp_package_metadata.package_synopsis_set_id OR 
(package_metadata.package_synopsis_set_id IS NULL AND 
temp_package_metadata.package_synopsis_set_id IS NULL))


signature.asc
Description: PGP signature

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

[Christopher Baines]

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


signature.asc
Description: PGP signature

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

[Christopher Baines]

Canan Talayhan  writes:

> 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/

Great, that fixes the issue with the revision comparison page.

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

I've gone ahead and merged this. I made some changes to the indentation,
I've generally just left that to Emacs, so that's effectively the
indentation style currently. I also tweaked the wording in the commit
message.

As for what to do next, it would be good to start looking at some stuff
that's more related to the project topic.

Part of the revision processing that I believe is quite slow and
hopefully can be improved is populating the package_metadata table. The
relevant lines in the job output look something like this:

  debug: Starting querying the temp_package_metadata
  debug: Finished querying the temp_package_metadata, took 1902 seconds

That output comes from the with-time-logging bit around here [1]. It's a
single query, generated by temp-table-select-query which is taking
around 30 minutes it seems, and I'd hope either the query can be made
faster, or some other faster way of doing what needs doing here can be
found.

1: 
https://git.savannah.gnu.org/cgit/guix/data-service.git/tree/guix-data-service/model/utils.scm#n333

insert-missing-data-and-return-all-ids is used in a few places, but this
specific issue arises when called from here [2].

2: 
https://git.savannah.gnu.org/cgit/guix/data-service.git/tree/guix-data-service/model/package-metadata.scm#n373

Making inserting package metadata faster is the overall goal, but I'd
suggest first just trying to reproduce the slow query outside of the
revision processing process. That way you'll be able to look at what the
query is doing and quickly test changes.

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).

Do let me know if you have any questions or get stuck, I'll hopefully be
around on IRC, and if I don't respond within a few minutes there, just
email me.


signature.asc
Description: PGP signature

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
> >> >> >
> >> >> > 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"))
> 

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

[Christopher Baines]

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
>> >> >
>> >> > 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 

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))
> >>   #: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 

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

[Christopher Baines]

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))
>>   #: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
 

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-data-service/web/dumps/html.scm
>   +++ b/guix-data-service/web/dumps/html.scm
>   @@ -21,8 +21,13 @@
>  #:use-module (guix-data-service web view html)
>  #:export (view-dumps))
>
>   +(define page-header "Database dumps")
>   +
>(define (view-dumps available-dumps)
>   +
>  (layout
>   +   #:title
>   +   page-header
>   #:body
>   `(,(header)
> (div
>   @@ -31,7 +36,7 @@
>   (@ (class "row"))
>   (div
>(@ (class "col-sm-12"))
>   -(h1 "Database dumps")))
>   +(h1 ,page-header)))
>
> Like the others, I'd probably put page-header inside 

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

[Christopher Baines]

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-data-service/web/dumps/html.scm
  +++ b/guix-data-service/web/dumps/html.scm
  @@ -21,8 +21,13 @@
 #:use-module (guix-data-service web view html)
 #:export (view-dumps))

  +(define page-header "Database dumps")
  +
   (define (view-dumps available-dumps)
  +
 (layout
  +   #:title
  +   page-header
  #:body
  `(,(header)
(div
  @@ -31,7 +36,7 @@
  (@ (class "row"))
  (div
   (@ (class "col-sm-12"))
  -(h1 "Database dumps")))
  +(h1 ,page-header)))

Like the others, I'd probably put page-header inside the view-dumps
procedure. Same goes for other places where it's outside.

  @@ -267,7 +279,7 @@
   (@ (class "col-sm-12"))
   (a (@ (href "/jobs"))
  (h3 "Jobs"))
  -(h1 "Queued jobs ("
  +(h1 ,page-header"("
   ,(length jobs-and-events)
   ")")))
 (div

I'd suspect the title here would be "Queued jobs(", I'd also put a space
between ,page-header the bit after it in the code.

  @@ -329,8 +341,13 @@
'())
jobs-and-events)

  +
   (define (view-job job-id query-parameters log)
  +  (define page-header (string-append "Job " job-id))
  +
 (layout
  +   #:title
  +   page-header
  #:body
  `(,(header)
(div

Most of the procedures are separated by one line, and I 

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-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 

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

[Christopher Baines]

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.


signature.asc
Description: PGP signature

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

[Christopher Baines]

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.


signature.asc
Description: PGP signature

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

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

[Maxime Devos]

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.


signature.asc
Description: This is a digitally signed message part

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: ).
>

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: ).
>
> > `((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
>

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

[Maxime Devos]

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: ).

> `((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


signature.asc
Description: This is a digitally signed message part