Fachschaft/sprechstunden-go
7
1
Fork 0
Code Issues 8 Pull requests Releases Activity

preparations for styling: refactor template rendering #1

Merged
Gonne merged 7 commits from styling into main 2022-09-26 10:46:05 +00:00
Conversation 2 Commits 7 Files changed 36 +345 -244
johannes commented 2022-09-21 20:50:31 +00:00
Member
Copy link

comments and review here, will resolve merge conflicts tomorrow

comments and review here, will resolve merge conflicts tomorrow
johannes added 3 commits 2022-09-21 20:50:32 +00:00
johannes commented 2022-09-21 21:06:15 +00:00
Author
Member
Copy link

https://www.alexedwards.net/blog/serving-static-sites-with-go

This link probably helps understanding which design I oriented at (one base template with specific blocks replaced per page)

https://www.alexedwards.net/blog/serving-static-sites-with-go This link probably helps understanding which design I oriented at (one base template with specific blocks replaced per page)
Gonne reviewed 2022-09-21 21:09:39 +00:00
Gonne left a comment
Owner
Copy link

Nice restructuring.

Nice restructuring.
controllers/templates.go Outdated
@ -22,0 +23,4 @@
// - name is the name of the template file inside templates/, without .html suffix.
// - data is passed through to the template
func (b *BaseHandler) serveTemplate(w http.ResponseWriter, name string, data any) {
full_name := "templates/" + name + ".html"
Gonne commented 2022-09-21 20:56:41 +00:00
Owner
Copy link

Here we should consider checking whether the name is restricted to something like [a-zA-Z0-9] to prevent directory traversal. Possibly it is sufficient to forbid / and/or .

Here we should consider checking whether the name is restricted to something like `[a-zA-Z0-9]` to prevent directory traversal. Possibly it is sufficient to forbid `/` and/or `.`
johannes commented 2022-09-24 21:44:42 +00:00
Author
Member
Copy link

This is not longer relevant, since all available templates are listed in one place inside our code and we do not parse filenames from anywhere else.

This is not longer relevant, since all available templates are listed in one place inside our code and we do not parse filenames from anywhere else.
controllers/templates.go Outdated
@ -22,0 +27,4 @@
// check that template exists
info, err := os.Stat(full_name)
if (err != nil && os.IsNotExist(err)) || info.IsDir() {
errMsg := fmt.Sprintf("Template %s nicht gefunden", full_name)
Gonne commented 2022-09-21 21:02:46 +00:00
Owner
Copy link

This could be an actual error using fmt.Errorf.
Furthermore I began logging in English. We should make that consistent.

This could be an actual error using `fmt.Errorf`. Furthermore I began logging in English. We should make that consistent.
johannes marked this conversation as resolved
controllers/templates.go Outdated
@ -22,0 +29,4 @@
if (err != nil && os.IsNotExist(err)) || info.IsDir() {
errMsg := fmt.Sprintf("Template %s nicht gefunden", full_name)
log.Println(errMsg)
w.WriteHeader(http.StatusNotFound)
Gonne commented 2022-09-21 21:03:53 +00:00
Owner
Copy link

I think, that StatusNotFound is for user supplied references (e.g. a course or a room id), but this should be an internal server error.

I think, that StatusNotFound is for user supplied references (e.g. a course or a room id), but this should be an internal server error.
johannes marked this conversation as resolved
controllers/templates.go Outdated
@ -22,0 +34,4 @@
return
}
tmpl, err := baseTemplate.ParseFiles(full_name)
Gonne commented 2022-09-21 21:08:17 +00:00
Owner
Copy link

Using this construction each template is parsed again and again, while it would be sufficient to parse them all on startup and later just execute them.

Using this construction each template is parsed again and again, while it would be sufficient to parse them all on startup and later just execute them.
johannes commented 2022-09-24 21:46:18 +00:00
Author
Member
Copy link

Fixed (and spent too much time implementing this ^^)

Fixed (and spent too much time implementing this ^^)
controllers/templates.go Outdated
@ -22,0 +36,4 @@
tmpl, err := baseTemplate.ParseFiles(full_name)
if err != nil {
errMsg := fmt.Sprintf("Template %s konnte nicht geparst werden : %s", full_name, err.Error())
Gonne commented 2022-09-21 21:05:23 +00:00
Owner
Copy link

This could be an actual error using fmt.Errorf.
Furthermore I began logging in English. We should make that consistent.

Furthermore error wrapping is something cool I just learnt.

This could be an actual error using `fmt.Errorf`. Furthermore I began logging in English. We should make that consistent. Furthermore [error wrapping](https://blog.boot.dev/golang/wrapping-errors-in-go-how-to-handle-nested-errors/) is something cool I just learnt.
johannes commented 2022-09-24 21:51:40 +00:00
Author
Member
Copy link

Error wrapping looks nice. I started doing that.

In general it seems best to wrap errors which occur during runtime from the beginning of a http request handler, which then logs the error and returns an error page to the user. (I added an inline TODO about that)

Errors on startup however can just abort the whole program, like I implemented in main.go

Error wrapping looks nice. I started doing that. In general it seems best to wrap errors which occur during runtime from the beginning of a http request handler, which then logs the error and returns an error page to the user. (I added an inline TODO about that) Errors on startup however can just abort the whole program, like I implemented in `main.go`
controllers/templates.go Outdated
@ -22,0 +45,4 @@
// TODO: cache templates in parsed state, but not executed
err = template.Must(tmpl.Clone()).Execute(w, data)
if err != nil {
errMsg := fmt.Sprintf("Template %s konnte nicht ausgeführt werden : %s", full_name, err.Error())
Gonne commented 2022-09-21 21:05:57 +00:00
Owner
Copy link

This could be an actual error using fmt.Errorf.
Furthermore I began logging in English. We should make that consistent.

Furthermore error wrapping is something cool I just learnt.

This could be an actual error using `fmt.Errorf`. Furthermore I began logging in English. We should make that consistent. Furthermore [error wrapping](https://blog.boot.dev/golang/wrapping-errors-in-go-how-to-handle-nested-errors/) is something cool I just learnt.
johannes marked this conversation as resolved
johannes added 3 commits 2022-09-24 13:03:43 +00:00 
adapt error wrapping principle
johannes commented 2022-09-24 22:02:35 +00:00
Author
Member
Copy link

If you want to work on in go code, feel free to just merge this. I'll work on the actual template styling now.

If you want to work on in go code, feel free to just merge this. I'll work on the actual template styling now.
Gonne added 1 commit 2022-09-26 10:42:25 +00:00
Gonne force-pushed styling from 0a4245b094 to 9357ab1520 2022-09-26 10:44:29 +00:00 Compare
Gonne merged commit c9883b0d3f into main 2022-09-26 10:46:05 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Fachschaft/sprechstunden-go#1
No description provided.
Delete branch "styling"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?