Skip to content

Conversation

@scott2000
Copy link
Contributor

@scott2000 scott2000 commented Jan 27, 2025

The meaning of [noeol] might not be immediately clear to a user, so it would be good to document it on our documentation page for conflicts. We may also want to improve the conflict marker comments we use for this case in the future (possibly after receiving user feedback).

Follow up to #5186.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

Copy link
Contributor

@ilyagr ilyagr left a comment

Choose a reason for hiding this comment

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

This LGTM.

Thank you also for continuing to work on this. I'm sorry I haven't paid as much attention to this as I would have liked.

Please also give this a bit of time before merging, in case Yuya, Waleed, or others have comments.

The meaning of `[noeol]` might not be immediately clear to a user, so it
would be good to document it on our documentation page for conflicts. We
may also want to improve the conflict marker comments we use for this
case in the future (possibly after receiving user feedback).
Copy link
Contributor

@arxanas arxanas left a comment

Choose a reason for hiding this comment

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

Docs look good to me overall, thanks for adding this.

[no action required] Looking at the docs now, I think the square brackets and abbreviated "keyword'-ish name make me think that the conflict marker has semantic meaning. If it had just written (no trailing newline) like normal prose then I would have assumed that it was primarily for my benefit as a reader, rather than machine-meaningful content.

@scott2000 scott2000 added this pull request to the merge queue Jan 29, 2025
@scott2000
Copy link
Contributor Author

[no action required] Looking at the docs now, I think the square brackets and abbreviated "keyword'-ish name make me think that the conflict marker has semantic meaning. If it had just written (no trailing newline) like normal prose then I would have assumed that it was primarily for my benefit as a reader, rather than machine-meaningful content.

That's a good point. I think I'll merge this PR, and then make a separate PR that makes the wording a bit more clear as you suggested.

Merged via the queue into jj-vcs:main with commit 95df920 Jan 30, 2025
19 checks passed
@scott2000 scott2000 deleted the push-trvpswuzqtrn branch January 30, 2025 00:04
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.

3 participants