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

Merge default data language data in config.toml #1918

Closed
Keats opened this issue Jul 11, 2022 · 9 comments
Closed

Merge default data language data in config.toml #1918

Keats opened this issue Jul 11, 2022 · 9 comments
Assignees
Labels
done in pr Already done in a PR enhancement

Comments

@Keats
Copy link
Collaborator

Keats commented Jul 11, 2022

Right now we can put the data in 2 different places for the default language in config.toml: the base file and language.DEFAULT_LANGUAGE. Those are not currently merged and zola will just take language.DEFAULT_LANGUAGE data if present, resulting in missing data and hard to diagnose error. See https://github.com/justint/papaya/blob/44be3a1b39a8bad5b33e55e8f6de8953d0ca2e20/config.toml for an example

@justint
Copy link
Contributor

justint commented Sep 14, 2022

Hi @Keats and @peterkelm, just a heads up that I've since updated papaya to declare the taxonomies in both the global section and [languages.en] section of the config.toml, see: justint/papaya@8566ba1

Is that the right way to go about declaring the taxonomies for multilingual sites?

@Keats
Copy link
Collaborator Author

Keats commented Sep 14, 2022

It's probably better to keep it at the root level for now since you can end up missing data otherwise.

This issue is a good one if someone wants a pretty simple issue to contribute.

@getreu
Copy link
Contributor

getreu commented Sep 19, 2022

With v0.16.1 I get the error

...has taxonomy `categories:` which is not defined in config.toml

Version v0.15.3 compiles my site fine. Copying my categories definition under [languages.en]- as mentioned above - does not work for me.

@vcrn
Copy link
Contributor

vcrn commented Nov 18, 2022

I'd be happy to give this a shot!

Regarding the desired behavior: if the same variable is specified at the base section and under [language.DEFAULT_LANGUAGE], do we want base to override the latter or the other way around?

@Keats
Copy link
Collaborator Author

Keats commented Nov 19, 2022

That's a good question. I think it should probably error since that's a mistake.

@vcrn
Copy link
Contributor

vcrn commented Nov 20, 2022

Yeah, that sounds reasonable.

I think I have some idea of that might work. I haven't tried any fix yet, but I think this is do-able for me.

Could you put me as assignee on this issue?

@vcrn
Copy link
Contributor

vcrn commented Nov 21, 2022

There are some fields of LanguageOptions that make it slightly less straight-forward to merge, unlike the ones of the types Option, bool or String.

  • The field search: Search contains a number of different fields of types Option, bool and String, and one Enum that has a default type. The field search has a default implementation.
  • The field translations: HashMap<String, String>. Empty by default.
  • The field taxonomies: Vector<TaxonomyConfig>, where TaxonomyConfig consists of fields only of types Option, String and bool. Empty by default.

Similar manners exist for handling these more complex types:

  • The simple one would be to raise an error when both fields are not empty, not equal to each other, or not equal to the default implementation. Otherwise merge.
  • The more rigorous one would be to in turn compare the contents of these fields and apply the same logic of merging them as when merging the fields of LanguageOptions.

I'm leaning toward the simple approach for all of these fields and stopping the user at that point, since they should probably specify their config.toml in another way. What do you think?

Also, I suspect that if we implement the functionality of Zola raising an error during merge conflict, it will break a substantial amount of multilingual sites where the user was not fully aware of this behavior when writing the config. What do you say about simply raising a warning at merge conflict to begin with, merging by giving the language section variables priority over the base (since that's how it's been working so far), and see how that works out?

@Keats
Copy link
Collaborator Author

Keats commented Nov 22, 2022

Thinking about it more, we should probably have a warning either way if the section for the default language is defined and some fields are defined at the root of the config.toml.
And an error as you mention if it's filled in both but different.

What do you say about simply raising a warning at merge conflict to begin with, merging by giving the language section variables priority over the base (since that's how it's been working so far), and see how that works out?

We can do that yes. Although since Zola releases are not super frequent, I think it might enough to document this change in the changelog and keep the error.

@vcrn
Copy link
Contributor

vcrn commented Nov 24, 2022

Yeah, I'll go ahead with the approach that you're suggesting. I've started writing the first test, so hopefully I'll have something up for review in the coming days.

@Keats Keats added done in pr Already done in a PR and removed help wanted good first issue labels Jan 17, 2023
normful added a commit to normful/blog that referenced this issue Jan 22, 2023
…using papaya theme

What:

Copies the taxnomies config to all languages:

Why:

Fixes this error:

```
[I] norman@cyan ~/code/blog (main)
❯ zola build
Building site...
Error: Failed to build the site
Error: Page `/Users/norman/code/blog/content/projects/project-1/index.md` has taxonomy `categories` which is not defined in config.toml
```

Discussion: https://zola.discourse.group/t/zola-0-15-3-issue-not-all-pages-sections-have-their-language-and-translations-set-properly/1385
Open issue: getzola/zola#1918
@Keats Keats closed this as completed Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
done in pr Already done in a PR enhancement
Projects
None yet
Development

No branches or pull requests

4 participants