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

semantic conditions for ground graphs damaged #56

Open
pfps opened this issue Dec 3, 2024 · 11 comments
Open

semantic conditions for ground graphs damaged #56

pfps opened this issue Dec 3, 2024 · 11 comments
Labels
needs discussion Proposed for discussion in an upcoming meeting spec:bug Change fixing a bug in the specification (class 3) –see also spec:substantive spec:substantive Change in the spec affecting its normative content (class 3) –see also spec:bug, spec:new-feature

Comments

@pfps
Copy link
Contributor

pfps commented Dec 3, 2024

PR #52 has changed the semantic conditions for ground graphs by changing the scope of the otherwise parts of clauses. This needs to be undone. The best way to do this is to undo PR #52, which should not have included substantive changes.

@pfps pfps added needs discussion Proposed for discussion in an upcoming meeting spec:bug Change fixing a bug in the specification (class 3) –see also spec:substantive spec:substantive Change in the spec affecting its normative content (class 3) –see also spec:bug, spec:new-feature labels Dec 3, 2024
@gkellogg
Copy link
Member

gkellogg commented Dec 3, 2024

Undoing that PR would be regressive at this point, IMO. Best would be a PR to add an update to revert the wording of those clauses.

(BTW, I suspect #52 was considered to be clear to merge, as sufficient time went by and there were no "changes required" notes in the approvals).

@pfps
Copy link
Contributor Author

pfps commented Dec 3, 2024

It is a very bad idea to have a PR that advertises as only making non-substantive changes but that also makes substantive ones.

Also, what other substantive changes are hidden in the supposedly non-substantive changes? I don't know.

Also, this is yet another PR that was applied over the written objections of a WG member.

@TallTed
Copy link
Member

TallTed commented Dec 3, 2024

[@gkellogg] (BTW, I suspect #52 was considered to be clear to merge, as sufficient time went by and there were no "changes required" notes in the approvals).

But there were at least two people (@pfps and me) saying, "I cannot review this PR properly" because it touched too many lines in too many ways. These were not "changes required" with suggestions, but to my mind, they certainly were "changes required" because of unreviewability.

@gkellogg
Copy link
Member

gkellogg commented Dec 3, 2024

[@gkellogg] (BTW, I suspect #52 was considered to be clear to merge, as sufficient time went by and there were no "changes required" notes in the approvals).

But there were at least two people (@pfps and me) saying, "I cannot review this PR properly" because it touched too many lines in too many ways. These were not "changes required" with suggestions, but to my mind, they certainly were "changes required" because of unreviewability.

My point is that, if you're not satisfied with the state of the PR, whether you have specific suggestions or not, reviewing as "changes required" (actually "Request changes") will prevent the PR from being merged. I'm not defending that it was merged somewhat arbitrarily, but that we can better use the GitHub tools to manage these things.

@pfps
Copy link
Contributor Author

pfps commented Dec 3, 2024

Was "Request changes" the decided-on way to register concerns? If not, then any comment expressing concerns should have stopped merging.

@domel
Copy link
Contributor

domel commented Dec 3, 2024

As usual on github.

@TallTed
Copy link
Member

TallTed commented Dec 3, 2024

As usual on github.

I am not feeling like these glib, fragmentary one-liners are appropriate to address the concerns @pfps and I have raised here and in #55 and #52.

I don't think the W3C nor this group has any policy stating "we follow usual github practice". Even if such existed, my experience on GitHub is that every project has its own "usual". In other words, there is no "as usual on github".

There is some "as usual in W3C", and that generally includes checking in with anyone who has voiced significant concern with an Issue or a PR, to confirm that they're OK with the PR before it gets merged, or OK with closing the Issue with whatever handling (PR, won't-fix, next version, etc.). I don't feel like that "as usual in W3C" has been followed, here.

@domel
Copy link
Contributor

domel commented Dec 3, 2024

So let's recall a few facts: 1. My PR addressed several key incorrect or at least non-semantic uses of HTML elements, in my opinion. 2. You proposed many additional changes, not all of which were important, but I implemented them anyway. 3. In the last few weeks, you haven't reviewed the changes I asked you to make, or you did but didn't feel like clicking the button. 4. The only change in the content was what Gregg changed in the next PR. 5. You still have a grudge against me because you cannot use tools to verify whether Gregg and I are lying to you. 6. You propose to revert the PR and keep the HTML code bad, and in your opinion, this is the best solution.

Are there any other points I've forgotten?

@TallTed
Copy link
Member

TallTed commented Dec 4, 2024

  1. My PR addressed several key incorrect or at least non-semantic uses of HTML elements, in my opinion.

Sure. I don't think I argued with any of your HTML changes. I do think I suggested corrections to some number of errors, among which I include the addition of some number of tags that you had omitted, though your commit notes suggested that they had been applied to the entire document. I don't recall whether I offered any corrections to other HTML errors; I'll presume that all your HTML, where it was not omitted, was perfect.

You seem not to be taking my comments and suggestions in the spirit intended. I apologize if my wording has communicated anything other than belief that your efforts have been in good faith, and my own suggested changes are in service of further improvement to the spec.

  1. You proposed many additional changes, not all of which were important, but I implemented them anyway.

"Importance" is in the mind of the reader. My suggested changes are always in service of making the spec easier on the unfamiliar reader (while hopefully not making it harder on the familiar reader), and more likely to be understood as meaning what we intended it to mean. I appreciate that you applied my change requests to the lines you were already touching (or very nearby). It is far faster and easier to make such change requests on an existing PR, than it is to make a distinct PR, even as easy as GitHub has made things through their web interface (which is my primary tool).

  1. In the last few weeks, you haven't reviewed the changes I asked you to make, or you did but didn't feel like clicking the button.

Forgive me. I am actively participating in a number of W3C Working Groups and W3C Community Groups, among other similar efforts, amidst my other $dayjob responsibilities. I tend to have 250+ GitHub notifications after a weekend, and 100+ daily. Sometimes something gets inadvertently clicked as "done" when it hasn't been. A gentle request that I look at something in particular is usually enough to get it back into my queue.

  1. The only change in the content was what Gregg changed in the next PR.

Please, whenever possible, refer to PRs and Issues by their number or URL, instead of or in addition to such relative references as "next", "last", "previous", etc. I am not sure which you reference here, so I cannot address this materially.

  1. You still have a grudge against me because you cannot use tools to verify whether Gregg and I are lying to you.

I am quite sure I've never accused either you or Gregg of lying to me. Typos and other errors happen. Likewise overlooking some things that were meant to have been changed in the current batch (e.g., omitting some <em> tags that were meant to have been added) is natural, especially in large files like this, and especially when making changes that touch so much, where those changes are intermingled with each other. I believe reviews by me and others are meant to catch such errors, which I certainly class as unintentional.

I don't tend to carry grudges. I try to treat everyone as acting in good faith. I do get frustrated when my own good faith efforts are not treated as such. I do get frustrated when people choose to work in a collaborative space in a fashion which is easiest for them, without apparent concern about whether that way is easier or harder for other collaborators. I hope my use of GitHub's web interface does not make things harder for you.

  1. You propose to revert the PR and keep the HTML code bad, and in your opinion, this is the best solution.

I don't believe I've ever advocated "keeping the HTML code bad". I've certainly advocated for making the 2000 lines of HTML and other changes submitted as #52 into phases/chunks that are more reasonably reviewable, toward getting them all merged in, and thereby making the HTML code "good" (or at least, "better").

@pfps
Copy link
Contributor Author

pfps commented Dec 11, 2024

I have several problems with PR #52. The changes were not as described. It had many changes. It was very hard to determine just what was changed and how.

The bottom line is that I no longer have confidence that the current version of RDF Semantics corresponds to what has been decided by this and previous working groups. There are two ways that I could again gain confidence in the correspondence - reverting PR #52 and a careful line-by-line examination of the document. I strongly vote for the former.

@w3cbot
Copy link

w3cbot commented Dec 19, 2024

This was discussed during the #rdf-star meeting on 19 December 2024.

View the transcript

semantic conditions for ground graphs damaged 2

<james> was there not a recent discussion from the semantics group the a term should be permitted in the subject position?

gkellogg: did we discuss this last week?
… pchampin was to re-apply changes.

pchampin: yes.

ora: if pchampin hasn't done this yet, maybe this should be deferred.

pfps: sorry. audio issues.
… yes, this was discussed last week.
… maybe the thing to do is to undo the PR for now, and pchampin can put in a new PR later.
… I don't know how to undo a PR.
… (that's been merged)

gkellogg: can reset the HEAD rather than a commit that undoes it.

AndyS: git revert. will still be in the log.

pfps: somebody who understands that better than I should do it.

pchampin: can git-revert do multiple commits at the same time?

AndyS: one at a time. only talking about HEAD of main, though.
… can re-organize locally and repush. will have to take branch protection off.

pchampin: I will figure it out. Unless somebody objects to a force-push...
… any solution is OK unless somebody objects.

ora: undo/revert. then re-apply the editorial?

pchampin: then a new PR with as well-documented as I can editorial changes.

AndyS: I was wrong. It's only the merge request that's the HEAD. there are a lot of commits.
… I thought they had been squashed, but they haven't.
… back to May 2nd.

<ora> PROPOSAL: pchampin to revert/undo the merge, then create new PR with select editorial changes

<pfps> +1

<ora> +1

<gkellogg> +1

<TallTed> +1

<gtw> +1

<niklasl> +1

<ktk> +1

<james> +1

<pchampin> +1

<AZ> +1

gkellogg: if each of multiple selections is committed individually, creates a big history. instead, use batch mechanism from file view to reduce number of commits.
… alternatively, when merging, do a squash commit to leave a clean trail.

RESOLUTION: pchampin to revert/undo the merge, then create new PR with select editorial changes w3c/rdf-semantics#56

<gb> Issue 56 semantic conditions for ground graphs damaged (by pfps) [needs discussion] [spec:bug] [spec:substantive]

AndyS: or squash locally, force-push to the branch.

AndyS: should be w3c/rdf-semantics#52

<gb> CLOSED Issue 52 Comment for rdf:PlainLiteral in the specs (by domel) [needs discussion]

pchampin: I can do the substitute.

<AndyS> +1 and thanks to Dominik for the work he's done.

<pchampin> s|RESOLVED: pchampin to revert/undo the merge, then create new PR with select editorial changes w3c/rdf-semantics#56

<pchampin> |RESOLVED: pchampin to revert/undo the merge, then create new PR with select editorial changes w3c/rdf-semantics#52

<gb> MERGED Pull Request 52 Better HTML (by domel) [spec:editorial]

ktk: who will do the new PR?

pchampin: I will.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion Proposed for discussion in an upcoming meeting spec:bug Change fixing a bug in the specification (class 3) –see also spec:substantive spec:substantive Change in the spec affecting its normative content (class 3) –see also spec:bug, spec:new-feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants