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 typo: allowedNetworks > allowedNetwork #832

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Catfriend1
Copy link
Contributor

No description provided.

@tomasz1986
Copy link
Contributor

tomasz1986 commented Oct 14, 2023

This one seems weird. In config.xml, the option is indeed called allowedNetwork. However, in the Advanced Configuration (and many other places in the code, e.g. https://github.com/search?q=repo%3Asyncthing%2Fsyncthing+allowednetworks&type=code), it's the plural form allowedNetworks. Why is it so?

@er-pa
Copy link
Member

er-pa commented Oct 15, 2023

The error lies more in the limited description of https://docs.syncthing.net/users/config.html#config-option-device.allowednetwork than in the plural form elsewhere. Just be more explicit in how it works, would suffice.

Under the hood the multiple networks become multiple single <allowedNetwork> entries in the config, but from a user's perspective it's simply just a list of elements and not limited to one single network. So removing the plural form here is actually faulty, as you're actually allowed to supply multiple networks here.

Editing the advanced setting is basically making a list of single allowedNetwork entires in the config. So it's both pretty much correct.

@calmh
Copy link
Member

calmh commented Oct 15, 2023

This discrepancy is there in all multiple-valued config items, like listenAddresses which is called that in the GUI and in the JSON (where it's a list of strings) but in the XML is repeated listedAddress tags, and maybe could use some clarification.

It's essentially the difference between listenAddresses: ["a", "b"] and <listenAddress>a</listenAddress><listenAddress>b</listenAddress>.

@acolomb
Copy link
Member

acolomb commented Oct 15, 2023

The advanced settings GUI is deduced from the JSON representation. In XML, it's a list of <allowedNetwork> elements, but in JSON it gets serialized to an array named allowedNetworks. The latter is where the option name comes from. The link to the config docs page does work, because both plural and singular forms are accepted as aliases.

Now we need to be clear on whether the heading on this page refers to the GUI label in the advanced settings modal (Allowed Networks), or to the XML description.

@acolomb
Copy link
Member

acolomb commented Oct 15, 2023

Actually I think it would be better to write this page from the perspective of the GUI. The configuration page is the one which describes the intricate details of the XML file, and links to the other pages when there is more to write about the topic, like in this case. But those users who discover this page directly will probably not be sifting through XML, but looking for an explanation what the thing in this "advanced settings" dialog is doing. Even if they just clicked on the question mark icon and came here through the link from the configuration page.

Thus, the current text is fine and no changes needed IMO. If anything add a remark on the config.rst page that the XML element is repeated instead of containing a comma separated list. For those who actually need to work with the XML format.

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.

5 participants