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

Fixes some premature changes from PR #55. #57

Closed
wants to merge 1 commit into from
Closed

Conversation

gkellogg
Copy link
Member

@gkellogg gkellogg commented Dec 3, 2024

There were a couple of areas where the changes were significant that are rolled back here.

Fixes #56.


💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Dec 3, 2024, 5:43 PM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL

Timed out after waiting 30000ms

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@gkellogg gkellogg requested review from domel, TallTed and pfps December 3, 2024 17:42
@TallTed
Copy link
Member

TallTed commented Dec 3, 2024

I'm fairly certain that these are not (all of) the changes in #55 that @pfps found problematic. Sorry, mental hiccup, this was about #52 (which has significant impact on the content being changed by #55).

(That said -- ) I'm more certain that I will be suggesting these changes again, whether as my own PR or as change requests on this or another PR. The phrasing of the complex if x then z if y statements being applied (restored) by this PR is much less clear to the reader than the if x and y then z phrasing I previously suggested and which is seen as the content now being changed. None of my textual edits changed the meaning of the text; they just restructured the presentation of the complex if/then statements.

@gkellogg
Copy link
Member Author

gkellogg commented Dec 3, 2024

I agree that the wording, which was restored in the PR, is problematic. But looking at the HTML diff, these were the only substantive changes i saw. The point was to get back to the content before the PR, as the objection was that both the amount of markup change and the different wording of the semantic conditions shouldn’t have been done at the same time.

@pfps
Copy link
Contributor

pfps commented Dec 3, 2024

'If ... then ... if ... otherwise ..." is a traditional way of stating a particular kind of conditional. It has a different meaning than "If ... and ... then ... otherwise ..."

@pfps
Copy link
Contributor

pfps commented Dec 3, 2024

I agree that the wording, which was restored in the PR, is problematic. But looking at the HTML diff, these were the only substantive changes i saw. The point was to get back to the content before the PR, as the objection was that both the amount of markup change and the different wording of the semantic conditions shouldn’t have been done at the same time.

The right way to get back to the undamaged state is to revert the "editorial" PR, create a truly editorial PR, and apply that.

Copy link
Contributor

@pfps pfps left a comment

Choose a reason for hiding this comment

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

Revert #52 first.

@afs
Copy link

afs commented Dec 4, 2024

https://pr-preview.s3.amazonaws.com/w3c/rdf-semantics/52/e29d0aa...84888af.html shows the non-HTML differences. It is readable. This was discussed at the WG meeting of 2024-11-21.

What non-HTML format issues are there other than the two items reverted by this PR (#57)?

@TallTed
Copy link
Member

TallTed commented Dec 4, 2024

"https://pr-preview.s3.amazonaws.com/w3c/rdf-semantics/52/e29d0aa...84888af.html shows the" 404 "non-HTML differences."

It appears to me that there are many HTML differences within that preview, primarily but not only 100+ added <em>...</em> wrappers. One large (non-<em>) example is change 231 of the 404.

@afs
Copy link

afs commented Dec 4, 2024

example is change 231 of the 404.

Thee layout seems to be an artefact of the diff tool

The current editors working draft shows the PR input appearance:
https://www.w3.org/TR/rdf12-semantics/#RDFS_axiomatic_triples

@gkellogg
Copy link
Member Author

gkellogg commented Dec 4, 2024

example is change 231 of the 404.

Thee layout seems to be an artefact of the diff tool

The current editors working draft shows the PR input appearance: https://www.w3.org/TR/rdf12-semantics/#RDFS_axiomatic_triples

Yes, a visual examination shows just a couple of changes in white-space. HTML diff is not a perfect tool, particularly when it comes to pre blocks.

Unless there's a concern about the legitimacy of some HTML markup, I think it would be better to confine objections to the rendered content. Sometimes, HTML diff is enough, other times you need to bring up windows next to each other to visually compare.

In a perfect world, changes to markup/structure would not introduce any other changes in content, but it ends up happening due to human nature and the perceived block of needing to have a PR merged before those additional changes can be made. With our deliberate delays in merging PRs, this can have the affect of stringing things out over quite a long time.

@pfps
Copy link
Contributor

pfps commented Dec 5, 2024

On a quick perusal of PR #52 HTML diff, I see at least 19 areas where the changes are not just better HTML. When I did a very careful examination, the second one of these made substantive changes to a core definition of the RDF semantics. I have no confidence that the other non-HTML change areas did not also make substantive changes.

So, the only acceptable way forward for me is to revert PR #52 and start over from a state that is known to be good.

@afs
Copy link

afs commented Dec 5, 2024

to a core definition

Peter - I don't understand what part of the document your comment is referring to.
Please give the document location(s).

@pfps
Copy link
Contributor

pfps commented Dec 6, 2024

See #52 (comment)

@pfps pfps added the spec:bug Change fixing a bug in the specification (class 3) –see also spec:substantive label Dec 12, 2024
@gkellogg
Copy link
Member Author

Closing, as the basis of this PR is being reverted.

@gkellogg gkellogg closed this Dec 19, 2024
@gkellogg gkellogg deleted the fix-pr-55 branch December 19, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:bug Change fixing a bug in the specification (class 3) –see also spec:substantive
Projects
None yet
Development

Successfully merging this pull request may close these issues.

semantic conditions for ground graphs damaged
5 participants