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

Expose String.validate_identifier method #67701

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dalexeev
Copy link
Member

Expose the validate_identifier method to the public API since validate_node_name is already exposed. validate_identifier can be useful for generating identifiers in EditorPlugins, EditorScripts, and other user code.

Changed behavior of validate_identifier to be consistent with validate_node_name and usability:

  1. Invalid characters are removed, not replaced with _.
  2. Numeric strings are prefixed with _ rather than replacing the first digit with _.

Changed/added test cases and documentation.

Fixed comment: #67018 (comment).

@dalexeev dalexeev requested review from a team as code owners October 21, 2022 06:41
@dalexeev dalexeev force-pushed the string-validate-identifier branch 3 times, most recently from 58cb3f2 to 537d96f Compare October 21, 2022 07:51
@Chaosus Chaosus added this to the 4.x milestone Oct 21, 2022
@akien-mga
Copy link
Member

akien-mga commented Jan 18, 2023

This implementation sounds much slower as you're recreating a string char by char each time, when in the vast majority of cases identifiers are valid and should simply be reused as is.

Could you benchmark it on both valid and invalid identifiers of different lengths?

@dalexeev
Copy link
Member Author

dalexeev commented Jan 18, 2023

Perhaps it's better to copy valid substrings, like in String::_camelcase_to_underscore (link)?

String new_string;
int start_index = 0;

for (int i = 1; i < this->size(); i++) {
    // ...

    if (/* current char is invalid */) {
        new_string += this->substr(start_index, i - start_index);
        start_index = i;
    }
}

new_string += this->substr(start_index, this->size() - start_index);

If the string is already a valid identifier, then it will be copied entirely 1 time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants