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 HTML/JavaScript > >> that is malicious and shouldn't be included in the page. A similar > >> approach probably needs taking for the title. > > This stuff above still looks the same to me, although part of my > analysis was wrong, as the type of an invalid parameter is a record, so > the page just breaks if the parameters are invalid (which I guses is > better than what I was describing). > > Anyway, I think this still needs fixing. > > >> @@ -419,14 +424,18 @@ > >> '(span (@ (class "text-success glyphicon glyphicon-plus > pull-left") > >> (style "font-size: 1.5em; padding-right: 0.4em;")))) > >> > >> + (define page-header "Comparing derivations") > >> + > >> (layout > >> + #:title > >> + page-header > >> #:body > >> `(,(header) > >> (div > >> (@ (class "container")) > >> (div > >> (@ (class "row")) > >> - (h1 ,@(let ((base-commit (assq-ref query-parameters > 'base_commit)) > >> + (h1 ,(let ((base-commit (assq-ref query-parameters > 'base_commit)) > >> > >> Why's the @ being removed here? > > This is still being removed. > > @@ -435,7 +439,7 @@ > " and " > (a (@ (href ,(string-append "/revision/" > target-commit))) > (samp ,(string-take target-commit 8) "…"))) > - '("Comparing derivations"))))) > + '("Comparing derivations"))))) > (div > (@ (class "row")) > (div > > Watch out for unwanted indentation changes you're making, I think the > previous indentation was better. > > @@ -256,7 +264,12 @@ > recent-events))))))))) > > (define (view-job-queue jobs-and-events) > + (define page-header (string-append "Queued jobs (" > + (number->string (length jobs-and-events)) ")")) > + > (layout > + #:title > + page-header > #:body > `(,(header) > (div > > For the page-header here, I'd indent the code like so: > > (define page-header > (string-append "Queued jobs (" > (number->string (length jobs-and-events)) > ")")) > > The main change being moving the code off the (define ... line. If it's > a one line thing that can fit, I think it's fine to have as one line, > but otherwise, I think this makes it easier to read. > > The other thing to fix before merging is the commit message. The current > one is a bit odd, and the first line is too long. >