-
Notifications
You must be signed in to change notification settings - Fork 68
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
Wrapped utility functions in namespaces #1320
Conversation
If this is accepted, I can create a public Gist listing all functions contained in each namespace. Then we can attach that to our CHANGELOG to ease the migration to v3.0.0 (we should do the same with the new package exports). |
Please also rename the tests in |
I saw a difference in using the namespaces. On one side you are prefixing all olds methods with „XXXUtils.abc“ and one the other side you are deconstructing the namespace with „const { abc } = XXXUtils“. Minor thing, but why is this like that? Having the deconstructor has the advantage that all changes concentrate in the header of each file. |
These are two options that you can use as a consumer of the library. Both are ok – it depends on how often you use the functions and whether you want to make it explicit where they come from at the usage site. |
|
||
private isStarting = true; | ||
startRegex: string; | ||
private endRegexStack: string[] = []; | ||
startRegexp: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines are an inconsistent renaming, I think. In C# we call regular expressions "Regex". In JS we call them "RegExp", but not mix that with C# to "Regexp". See the class name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's why I propose to rename all instances to "RegExp" to be in line with the JavaScript RegExp object.
However, the TerminalRegExpVisitor class is not exported, but is private to the regexp-utils module, so those changes are not relevant for the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is private. But I would not argument like that 'because it is private, we do not have to be consistent (it even does not contribute to the API)'. You would also not start to use identifiers like i, j, k for for-loops, just because the code is not so public, like other code.
But I also see that it is just a very minor thing. I can live with this, but my consistency alert is then popping up when I see this again ^^*.
Apart from the REGEX(P) renaming, everything else looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I was always a bit unhappy about the distinction of CST and AST related functions that were exported from langium
directly. I think this change clears things up nicely and aligns everything with the existing UriUtils
.
This is a proposal following up on #1260.
AstUtils
CstUtils
GrammarUtils
RegExpUtils
ValueConverter
utils
to be consistent (we already exportedUriUtils
, so we should follow that)Using the method described in #1260, this change reduces the number of top-level exported functions from the main export of
langium
by 67! That number will be further reduced once #1258 is merged.