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

Clarify and expand descriptions of NonFiles #1734

Merged
merged 4 commits into from
Dec 22, 2024

Conversation

EliahKagan
Copy link
Member

This is a minor documentation revision, to make clearer what a NonFile/"non-file" is, as discussed in #1730 (comment). At least for now, they are named the same, but the term is more specifically defined.

A couple of possible other names that it could be changed to:

  • Other as suggested in #1730 (comment). As noted, this is natural. But it is not specific. If Git uses this as a technical term, as suggested, then it should probably be used. I was unable to find it in a cursory search of git/git on GitHub, though. I didn't find anything for "NonFile" and everything I found for "non-file" was in phrases like "non-file mailmap," "non-file remote," "non-file backend," and "non-file argument."
  • NonTrackable, or variations such as Untrackable and NotTrackable - This idea is from a commit message in a test that refers to the condition of not being a NonFile as that of being a "trackable file." I think this is also intuitive when considered abstractly, and accurate. But I'm not sure if it would be intuitive in practice.

Instead of changing it to one of those here, I've just clarified and expanded the documentation. This is with the idea that, with the documentation adjusted this way, it should be possible to figure out if any other name still confers a significant improvement in clarity.

There is another conceptually related change that I made to nearby documentation that is not really covered by this. I'll post a review comment on it so that if it is considered out of scope or unwanted then it can be undone.,

Discussed in:
GitoxideLabs#1730 (comment)

At least for now, they remain called `NonFile`s (and sometimes
referred to as "non-files" in text), but more specifically defined.
gix-dir/src/entry.rs Outdated Show resolved Hide resolved
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks so much for submitting this change, just in time and I think it's substantial!

Even though this PR goes for a minimal change to improve the documentation, the description also offered different variant names. Here I went for Untrackable which should help making clear what not to do when encountering such a directory entry.

gix-dir/src/entry.rs Outdated Show resolved Hide resolved
@Byron Byron enabled auto-merge December 22, 2024 08:07
Copy link
Member Author

@EliahKagan EliahKagan left a comment

Choose a reason for hiding this comment

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

Going along with changing NonFile to Untrackable, I think the term "non-file" in documentation comments should also be reworded, since without NonFile, it's not clear what "non-file" means.

Since I don't know what other changes you plan, I don't know if the suggestions I've made here are the way to go or not.

Edit: It looks like you've enabled auto-merge so I'm guessing these changes won't conflict with any further changes you already plan to make here. I'll see if I can get these in as another commit (via the suggestion batching feature), which I think will cancel auto-merge and let you review them, but if it doesn't work out then I can open another PR.

gitoxide-core/src/repository/clean.rs Outdated Show resolved Hide resolved
gix-dir/tests/dir/walk.rs Outdated Show resolved Hide resolved
gix-dir/tests/dir/walk.rs Outdated Show resolved Hide resolved
gix-status/src/index_as_worktree_with_renames/mod.rs Outdated Show resolved Hide resolved
This goes along with changing `NonFile` to `Untrackable`.
auto-merge was automatically disabled December 22, 2024 08:11

Head branch was pushed to by a user without write access

@Byron Byron enabled auto-merge December 22, 2024 08:14
@Byron
Copy link
Member

Byron commented Dec 22, 2024

A great catch, thanks! I was a bit sloppy quick here 😅.

EliahKagan

This comment was marked as resolved.

@Byron Byron merged commit ad6b9b6 into GitoxideLabs:main Dec 22, 2024
20 checks passed
@EliahKagan EliahKagan deleted the nonfiles branch December 22, 2024 08:33
@EliahKagan
Copy link
Member Author

I just realized the commit message in 154b21f somewhat overinflates its importance: it's not changing documentation comments, but instead a regular comment and debug and assertion messages. (Fortunately, since it's not really changing documentation, it's not a conventional commit with doc:, somewhat limiting the breadth of the confusion.)

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.

2 participants