Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix quotes in titles breaking graph json output #400

Closed
wants to merge 1 commit into from

Conversation

tjex
Copy link
Member

@tjex tjex commented Apr 3, 2024

resolves: #389

note titles with " marks in them, eg this is an "example" title, are breaking the output of zk graph --format json.

The culprit is the template for the json format. I can't exactly track down where that is set, but it would seem deeper from internal/core/notebook.go:336.

In any case, this fix manually sanitizes the quote marks as they're being returned to the Link field, for output to json. Link output within documents (i.e, inserting wiki/markdown links in documents is not broken as that is handled separately in internal/core/link_format.go).

The caveat is that this fix breaks an old tesh test that looks at json output.

However, it would seem that the old tesh test is not taking into account note titles with " marks.

Therefore, the failing test may be a sign of the test needing to be updated, rather than this fix being void?

In any case, this initial commit to the PR is effectively a draft. I'm looking for feedback about the above!

There is also [a discussion] in a previous issue which shows the development of the graph output, and also that quotes in the titles are breaking its output

It would seem that the old tesh test is not taking into account note
titles with `"` marks. Therefore, the failing test may be a sign of the
test needing to be updated, rather than this fix being void.
@tjex
Copy link
Member Author

tjex commented Apr 4, 2024

@mickael-menu Would it be ok to get some high level input from you on this one? As you had commented on the quotes breaking json before. Also the usage of this template functionality has got me a bit stumped:

func (n *Notebook) NewNoteFormatter(templateString string) (NoteFormatter, error) {
	templates, err := n.templateLoaderFactory(n.Config.Note.Lang)
	if err != nil {
		return nil, err
	}
	template, err := templates.LoadTemplate(templateString)
	if err != nil {
		return nil, err
	}

	linkFormatter, err := NewLinkFormatter(n.Config.Format.Markdown, templates)
	if err != nil {
		return nil, err
	}

	return newNoteFormatter(n.Path, template, linkFormatter, n.osEnv(), n.fs)
}

So I'm not that confident that I'm looking at this fix in the right way, and as the fix is changing the functionality that is also accessed by zk list, I'm wary of creating future bugs down the way.

@tjex
Copy link
Member Author

tjex commented Apr 7, 2024

accidentally pushed here from gh cli instead of my own repo..

@tjex tjex closed this Apr 7, 2024
@tjex tjex deleted the json-link-sanitize branch August 28, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zk graph --format json is not handling note titles with inner double quotes
1 participant