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

1592 Rework rules for selecting a layout #1596

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

michaelhkay
Copy link
Contributor

I've reworked the rules for selecting a layout. There's probably more to be done, but this is a start - feeback welcome. I'm marking this "revise" for the moment because I haven't finished it yet. There's no deliberate changing of the spec apart from fixing errors.

@michaelhkay michaelhkay added XQFO An issue related to Functions and Operators Editorial Minor typos, wording clarifications, example fixes, etc. Revise PR has been discussed and substantive changes requested labels Nov 21, 2024
Copy link
Contributor

@ChristianGruen ChristianGruen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelhkay I would need some more time to compare this with the previous approach. Do you think you could easily summarize what has mostly changed, apart from restructuring the layout selection rules?

specifications/xpath-functions-40/src/xpath-functions.xml Outdated Show resolved Hide resolved
</olist>
</item>
<item>
<p>If <code>every $e in $EE satisfies all-different($e/*/node-name())</code>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about (https://github.com/qt4cg/qtspecs/issues/1592#issuecomment-2493187896)…

Suggested change
<p>If <code>every $e in $EE satisfies all-different($e/*/node-name())</code>
<p>If <code>not(all-equal($EE/*/node-name()))</code>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the replacement expression is correct.

@michaelhkay
Copy link
Contributor Author

I think it's essentially (a) refactoring the way the layout selection rules are presented (fixing a couple of errors and omissions in the process), (b) adding notes on losslessness.

@michaelhkay michaelhkay removed the Revise PR has been discussed and substantive changes requested label Nov 29, 2024
@michaelhkay
Copy link
Contributor Author

In preference to the suggestion for defining a default layout, I have added an option disabled-layouts; for example if record layout is disabled, then an element that would have been formatted with record layout will typically be formatted with sequence layout instead. New tests needed for this option.

@michaelhkay michaelhkay added the Tests Needed Tests need to be written or merged label Nov 29, 2024
@ChristianGruen
Copy link
Contributor

ChristianGruen commented Nov 30, 2024

In preference to the suggestion for defining a default layout, I have added an option disabled-layouts; for example if record layout is disabled, then an element that would have been formatted with record layout will typically be formatted with sequence layout instead. New tests needed for this option.

I’m late, but wouldn't it be nicer to have an inclusive option (enable-layouts, use-layouts or just layouts) that by default includes all layouts? In practice, it might be easier to say which layout I want instead of excluding a layout and seeing if the right thing happens.

@michaelhkay
Copy link
Contributor Author

I’m late, but wouldn't it be nicer to have an inclusive option (enable-layouts, use-layouts or just layouts) that by default includes all layouts? In practice, it might be easier to say which layout I want instead of excluding a layout and seeing if the right happens.

I think the likely scenario is that the user will start by choosing the defaults, and if there's something in the output that they don't like, they will either want to provide a specific choice for one particular element, or if it affects a number of elements, they will want to suppress that specific layout.

@michaelhkay michaelhkay force-pushed the 1592-elements-to-maps-refinements branch from 0fa582a to 5386cf9 Compare December 3, 2024 14:19
@ndw
Copy link
Contributor

ndw commented Dec 3, 2024

The CG agreed to merge this PR at meeting 101.

@ndw ndw merged commit 380ec50 into qt4cg:master Dec 3, 2024
3 checks passed
@michaelhkay michaelhkay added Tests Added Tests have been added to the test suites Completed PR has been applied, tests written and tagged, no further action needed and removed Tests Needed Tests need to be written or merged labels Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Completed PR has been applied, tests written and tagged, no further action needed Editorial Minor typos, wording clarifications, example fixes, etc. Tests Added Tests have been added to the test suites XQFO An issue related to Functions and Operators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants