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

Update record names as reserved words/variables and new record definition syntax #64

Merged
merged 24 commits into from
Oct 22, 2024

Conversation

choptastic
Copy link
Contributor

Hello,

This is in reference to PR-7873 and is made on @RaimoNiskanen's recommendation.

@RaimoNiskanen, I did not include you as an author on the EEP, but since you implemented the new record syntax, you obviously deserve to be on it. I just didn't want to include you without your blessing first.

eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated
Comment on lines 15 to 16
words (`#if` vs `#'if'`) or words with capitalized first characters (`#Hello`
vs `#'Hello'`).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
words (`#if` vs `#'if'`) or words with capitalized first characters (`#Hello`
vs `#'Hello'`).
words (`#if` vs `#'if'`) or words with capitalized first characters (variable
names, for example `#Hello`vs. `#'Hello'`).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do like that this indicates that an unquoted word with a capitalized first character would indicate a variable name, but I find the specific wording without any qualifiers may imply that a record name can itself be a variable, rather than an atom that happens to have variable-style capitalization. Perhaps the term "atom with variable capitalization" with acronym "AVC" or something (or if there's a better term)? I know this is a little bike-shedding, but I'm concerned folks might interpret the use of "Variable name" to actually think the record name can be a variable.

If you think I'm off my rocker with this and I'm overthinking it, feel free to let me know :)

Copy link
Contributor

Choose a reason for hiding this comment

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

You have a point. Maybe just "like variable names". I interpret "atom with variable capitalization" as 'Hello' because quoting is how to create such an atom.
We should avoid inventing an acronym...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the phrasing here: dbfb39e

What do you think?

eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated Show resolved Hide resolved
@RaimoNiskanen
Copy link
Contributor

RaimoNiskanen commented Oct 9, 2024

There are also some hyphenations such as "use-case", "record-like", "single-quote" that I think should be written as separate words.

edit I just got enlightened that hyphen-composed words are used in adjective-like ways, by many writers of American English. So "record-like" above is by that OK.

The Markdown linter complains about mixing fenced code blocks (```erlang) with indented code blocks, that our mandatory header looks like / happens to be. Ignore those, and I will try to find how to disable that warning.

The other warnings should be fixed.

Other than that, and my inline comments, I think it looks good!

@RaimoNiskanen
Copy link
Contributor

@RaimoNiskanen, I did not include you as an author on the EEP, but since you implemented the new record syntax, you obviously deserve to be on it. I just didn't want to include you without your blessing first.

Although I finished the reference implementation, and thus more or less defined what this EEP should describe, I'd say you are still the author of the EEP, also despite me having a bunch of review comments...

@RaimoNiskanen
Copy link
Contributor

It seems I accidentally pressed a bad button and closed this... Oops! Sorry about that!

@RaimoNiskanen RaimoNiskanen reopened this Oct 9, 2024
@RaimoNiskanen
Copy link
Contributor

RaimoNiskanen commented Oct 9, 2024

I have updated the workflow configuration (on 'master') to disable Markdown linter rule MD046 - Code block style, and re-run it, but the unwanted warnings are still there. Can you try to rebase this PR to the latest 'master'?

eeps/eep-0072.md Outdated Show resolved Hide resolved
@michalmuskala
Copy link
Contributor

michalmuskala commented Oct 9, 2024

While I do understand the arguments for using unquoted atoms in record syntax and I consider it a simple and relatively small change, the change to record definition syntax is a major one and I believe should go through a bigger consideration.

In particular, the argument for symmetry, even with the change to definition syntax, still doesn't hold since things like is_record/2 guard won't work with unquoted names. So, you'd still need to know which names require quoting and which don't.

Furthermore, the EEP completely misses to discuss the impact on language by having 2 separate and parallel syntaxes for defining records. This will be confusing to learners and will inherently cause some split in "best practices" and across codebases. This might be OK if benefits outweigh this cost, but as it stands, I am not convinced this is true by what's contained in this EEP.

@RaimoNiskanen
Copy link
Contributor

In particular, the argument for symmetry, even with the change to definition syntax, still doesn't hold since things like is_record/2 guard won't work with unquoted names. So, you'd still need to know which names require quoting and which don't.

That is correct. The symmetry argument is only for definition vs. usage, which I think is the important ones. Everybody has to define a record to be able to use it.

Record names are atoms, and by is_record/2 and the other record meta functions this shines through... At some point in the language we will have to draw the line where it shines through, if we as this EEP proposes, relax the name rules for record usage.

I have always been a bit annoyed that record definition looks different from record usage, and from when defining a record type (where the name also can be unquoted, in this EEP:s reference implementation). Here I saw an opportunity to fix that, for convenience.

The old record definition style would be obsolete, and I don't think there is any reason to keep using it other than for backwards compatibility reasons.

@choptastic
Copy link
Contributor Author

edit I just got enlightened that hyphen-composed words are used in adjective-like ways, by many writers of American English. So "record-like" above is by that OK.

❤️ Fascinating! I love it. I absolutely adore learning things that I do every day or take for granted only to discover they are either local, regional, or national patterns. I had no idea that heavy hyphen use in this way was an American thing. I will review my overuse of hyphenated non-adjective-like words and revise accordingly :)

And thanks everyone for all the comments and discussions. I'm working on further revisions to the EEP and hope to have them pushed within the next hour or so!

eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated Show resolved Hide resolved
eeps/eep-0072.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@choptastic choptastic left a comment

Choose a reason for hiding this comment

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

Go ahead and ignore this comment - I misclicked something and it submitted it as review comments rather than a commit. I'll just merge one by one.

@choptastic
Copy link
Contributor Author

Alrighty, sorry for not commenting about my pushes last week. The days got away from me.

I've pushed a few small updates, but I think it's all good now, but let me know if you think I've missed anything big.

Copy link
Contributor

@RaimoNiskanen RaimoNiskanen left a comment

Choose a reason for hiding this comment

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

I think this looks fine!

@kikofernandez kikofernandez merged commit 97f8853 into erlang:master Oct 22, 2024
1 check passed
@choptastic choptastic deleted the record_updates branch October 23, 2024 16:48
@RaimoNiskanen
Copy link
Contributor

The OTP Technical Board meeting decided to postpone this proposal since we have other record improvement possibilities on the radar, and don't want to accidentally shut a syntactical door on such an improvement. We'll see for OTP 29 how these syntax extensions can play in, they will definitely be considered or incorporated.

@choptastic
Copy link
Contributor Author

@RaimoNiskanen, A related improvement might be to allowing the attribute names to follow the new proposed syntax as well, thereby allowing attribute names to be if, Variable, etc.

I haven't done anything to attempt to do this, but might that also be one of the things the team is considering? Given my work on Nitrogen is so heavily bound to records, I'm all kinds of intrigued by more improvements to working with records.

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.

8 participants