update txt metadata #82
Conversation
| var b bytes.Buffer | ||
| var titleDims []string | ||
| dt := time.Now() | ||
| issuedDate := dt.Format("01-02-2006") |
There was a problem hiding this comment.
Would it not make sense for this to include the time down to the minute?
| var b bytes.Buffer | ||
| func NewMetadata(m *dataset.Metadata, isCustom bool, filterOutputID, downloadServiceURL string) []byte { | ||
| if isCustom { | ||
| var b bytes.Buffer |
There was a problem hiding this comment.
Could be worth splitting custom dataset metadata and non custom metadata separated into two separate functions? That way easier to test logic
|
|
||
| "github.com/ONSdigital/dp-api-clients-go/v2/dataset" | ||
| ) | ||
|
|
||
| func NewMetadata(m *dataset.Metadata) []byte { | ||
| var b bytes.Buffer | ||
| func NewMetadata(m *dataset.Metadata, isCustom bool, filterOutputID, downloadServiceURL string) []byte { |
There was a problem hiding this comment.
Function could do with some unit tests
| if len(contacts) > 0 { | ||
| b.WriteString(fmt.Sprintf("Contact: %s, %s, %s\n", contacts[0].Name, contacts[0].Email, contacts[0].Telephone)) | ||
| for _, d := range m.Version.Dimensions { | ||
| titleDims = append(titleDims, d.Label) |
There was a problem hiding this comment.
Would it make sense to add a separator between each dimension label?
| b.WriteString(fmt.Sprintf("Issued: %s\n", issuedDate)) | ||
| b.WriteString(fmt.Sprintf("Language: %s\n", "English")) | ||
| b.WriteString("Distribution:\n") | ||
| for k, v := range m.Downloads { |
There was a problem hiding this comment.
Is it not possible to do the same for loop for non custom dataset? Like so:
for k, v := range m.Downloads {
b.WriteString(fmt.Sprintf("\tExtension: %s\n", k))
b.WriteString(fmt.Sprintf("\tSize: %s\n", v.Size))
b.WriteString(fmt.Sprintf("\tURL: %s\n\n", v.URL))
}
If yes, then can move this to a separate function and can be used by both sections
There was a problem hiding this comment.
I don't think this is possible as v.URL does not contain the correct url, therefore they need to be processed separately depending on the file type
| } | ||
| } | ||
| if m.Version.IsBasedOn != nil { |
There was a problem hiding this comment.
Variables that should be the same for custom or non custom dataset could be defined outside of if/else statement; saves the number of lines in the code
|
I'm going to merge to develop for now as this ideally needs to be ready for "go live" tomorrow. I will work on the requested changes afterwards |
What
Update txt download for BYO
Trello - https://trello.com/c/JkyoYKdQ/6077-update-txt-download-for-byo
How to review
Confirm changes match the requested changes in Trello ticket
Who can review
Anyone