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

Expand basendx comment #4283

Merged
merged 3 commits into from
Oct 15, 2024
Merged

Expand basendx comment #4283

merged 3 commits into from
Oct 15, 2024

Conversation

jkingdon
Copy link
Contributor

@jkingdon jkingdon commented Oct 12, 2024

This is to describe when to look at extensible structure indexes directly and when not to, in both set.mm and iset.mm. For further background see #3235 especially the comment #3235 (comment) which nicely summarizes the situation.

If people want to tackle this another way, speak up. But at least to my eyes the text we have now is pretty confusing about when we do and do not expect to look at these indexes.

This is to describe when to look at indexes directly and when not to, in
both set.mm and iset.mm.
Copy link
Contributor

@benjub benjub left a comment

Choose a reason for hiding this comment

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

That clarification is indeed welcome. Just a few remarks.

iset.mm Outdated Show resolved Hide resolved
iset.mm Show resolved Hide resolved
iset.mm Show resolved Hide resolved
Reword the text about the index being an implementation detail.

Use indices for the plural of index.
@avekens
Copy link
Contributor

avekens commented Oct 13, 2024

Fixes #3235

I think to fix #3235, we have to provide this comment (in a generalized form) at a central place, maybe in the header comment of section 7.1.1 or section 7.1.2 "Slot definitions" (here, maybe refering to the comment for ~basendx would be sufficient).

We can keep some of the details in the basendx comment but the section
header for the extensible structure section does discuss the numeric
indices and so we should mention the issue there and link to the more
detailed discussion.
@jkingdon
Copy link
Contributor Author

Fixes #3235

I think to fix #3235, we have to provide this comment (in a generalized form) at a central place, maybe in the header comment of section 7.1.1 or section 7.1.2 "Slot definitions" (here, maybe refering to the comment for ~basendx would be sufficient).

That's a good point. I had been meaning to look into that more so thanks for the reminder.

The header comment for the "Basic definitions" section (currently 7.1.1 in set.mm and a different number in iset.mm) already discusses the numeric indices and avoiding direct reference to them, so this seems like the place. I think putting a one sentence summary there with a pointer to basendx is probably the way to go (I can elaborate on why but the short summary is that our section headers, when we have them, tend to be pretty introductory and point to comments of specific theorems).

I've pushed up an addition intended to address this.

@avekens
Copy link
Contributor

avekens commented Oct 13, 2024

This PR is a very excellent first step in fixing #3235, so it can/should be accepted and merged now. To finish #3235, however, further discussions and actions seem to be necessary. I will provide corresponding remarks in #3235.

@jkingdon jkingdon merged commit d6bd32b into metamath:develop Oct 15, 2024
10 checks passed
@jkingdon jkingdon deleted the basendx branch October 15, 2024 06:07
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.

4 participants