Skip to content

Update code-gen tool for lowercase child name consistency #1908

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

Closed
wants to merge 1 commit into from

Conversation

Matejkob
Copy link
Contributor

@Matejkob Matejkob commented Jul 14, 2023

This PR introduces changes to the code-gen tool to ensure consistency with already generated code. This is a preparatory step before we start changing the names of all child elements to be lowercase - #1901.

In the #1901 issue description @ahoppen mention that

It's a little counter-intuitive that the name property of Child needs to be uppercase because AFAICT we are always using it as lowercase

After research, I've learned that we're using the name property of Child as uppercased in the following contexts:

  • as enum names e.g.: enum accessorSpecifierOptions: TokenSpecSet and them as types e.g.: ElseBody for IfExprSyntax,
  • to build names of "unexpected" nodes.

I guess now with this knowledge the question is: should we still lowercased values of the name property of each Child?

If we will decide to continue working on this issue here is the script I'm gonna use to change names:

import os
import re

def process_file(filename):
    # Open the file
    with open(filename, 'r+') as file:
        content = file.read()
        
        # Find all child declarations and replace the name field
        new_content = re.sub(r'(Child\(\s*name:\s*")([A-Z])', lambda match: match.group(1) + match.group(2).lower(), content)
        
        # Write the updated content back to the file
        file.seek(0)
        file.write(new_content)
        file.truncate()

def process_directory(directory):
    # Walk through all files in the directory
    for root, dirs, files in os.walk(directory):
        for file in files:
            # Only process .swift files
            if file.endswith('.swift'):
                process_file(os.path.join(root, file))

# Start the processing
process_directory('.')

Introduces changes to the code-gen tool to ensure consistency with already generated code. This is a preparatory step before we start changing the names of all child elements to be lowercase.
@Matejkob
Copy link
Contributor Author

@ahoppen I've drafted this PR to spark a discussion about what to do next with the issue

@ahoppen
Copy link
Member

ahoppen commented Jul 17, 2023

I just found out about the usage of capitalized name for the unexpected nodes yesterday as well. The way I would go about this that:

  • The child names as specified in SyntaxSupport should be lowercased because the name of the property is how we primarily think about the child
  • Child.name should be private (and will probably store the lowercase name from the Child initializer)
  • There’s varName (which I’m renaming to varOrCaseName in Improve modeling of Child in CodeGeneration #1914 to be consistent with Node.varOrCaseName), which should return the lowercase name
  • I’d introduce typeName, which returns a capitalized version of the child name (same as Child.name currently does). We should be able to use that in all the cases where we currently rely on Child.name being uppercase.

@ahoppen
Copy link
Member

ahoppen commented Aug 7, 2023

Did you close this PR by accident @Matejkob?

@Matejkob
Copy link
Contributor Author

Matejkob commented Aug 7, 2023

Nope, I didn't; just forgot to leave an explanation. 😅

So, here's why: PR #2011 introduces the last piece proposed by @ahoppen in the above discussion, allowing us to close #1901. In regard to this discussion, this draft became outdated and there isn't a single line of code in it that should be merged anymore.

After merging #2011, I'll publish a PR with lowercased child names.

@ahoppen
Copy link
Member

ahoppen commented Aug 7, 2023

Oh, OK. I didn’t read the title properly and thought this was the PR to lowercase the child names themselves. Thanks for clarifying.

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