Skip to content

Conversation

killerall
Copy link

Implemented getNumberOfSyntaxErrors for javascript runtime.

Signed-off-by: Rodrigo Antonio Godinho da Silva <rodrigo.antonio@totvs.com.br>
@ericvergnaud
Copy link
Contributor

Hi,
thanks for this, but why not simply use _syntaxErrors ? (the getter pattern is not recommended in JS)

@killerall
Copy link
Author

_syntaxErrors

Hello, for 3 reasons:
1-) Compatibility between java and javascript version.

2-) When using Lib with typescript, parser._syntaxErrors, I got this error:

"property '_syntaxErrors' does not exist on type"
Although at runtime, the value is accessible and correct.

3-) There is actually a long history of using dangling underscores to indicate “private” members of objects in JavaScript (though JavaScript doesn’t have truly private members, this convention served as a warning).

@ericvergnaud
Copy link
Contributor

ericvergnaud commented May 2, 2023

The Java field name is poorly named.
How about renaming (in JS only) _syntaxErrors to syntaxErrorsCount and exposing it in TS ?

@killerall
Copy link
Author

I can definitely do this PR with this change from _syntaxErrors to syntaxErrorsCount, but wouldn't that break the code of someone already using _syntaxErrors natively(JS)?

@ericvergnaud
Copy link
Contributor

You're right. Maybe add it as a "get" fn ?

@killerall
Copy link
Author

I'm sorry, but I'm not a javascript expert. Your suggestion is to do something like:

get syntaxErrorsCount() {
        return this._syntaxErrors;
    }

But I didn't find similar construction in Parser.js

@ericvergnaud
Copy link
Contributor

Indeed you're not seeing much of this because when I started the project 15 years ago it wasn't supported. But that's the way to go nowadays for simple getters.

@killerall
Copy link
Author

New PR with requested changes #4261

@killerall killerall closed this May 8, 2023
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