Skip to content
New issue

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

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

Already on GitHub? # to your account

PFPorts should rely on XML #123

Closed
Spartan322 opened this issue Oct 23, 2021 · 0 comments · Fixed by #130
Closed

PFPorts should rely on XML #123

Spartan322 opened this issue Oct 23, 2021 · 0 comments · Fixed by #130

Comments

@Spartan322
Copy link
Collaborator

Spartan322 commented Oct 23, 2021

Implementation PR: #130

PFPorts makes little sense in my eyes when we are using XML, I propose this format instead:

<PFPorts>
  <ssh /> <!-- this modifies the ssh default port -->
  <ftp Number="50" />
  <random_protocol Number="78">display name underscores are not spaces anymore</random_protocol>
  <!-- or -->
  <random_protocol Number="78" Display="display name underscores are not spaces anymore" />
  <!-- we could also do this even if its very verbose -->
  <random_protocol>
    <Number>78</Number>
    <Display>display name underscores are not spaces anymore</Display>
  </random_protocol>
  <ssh Remove="true" /> <!-- removes default port -->
</PFPorts>

While being a little more verbose, its guaranteed in the XML format, following the XML spec properly for definition behavior, and there is no special syntax to recall. If we really don't feel like introducing a breaking change and keep the old format, we can, it is slightly shorter in most cases, but for consistency I would say we should prefer this format.

@Spartan322 Spartan322 linked a pull request Oct 25, 2021 that will close this issue
Windows10CE added a commit that referenced this issue Nov 26, 2021
@Arkhist Arkhist closed this as completed Jan 30, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants