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

BUPH-117 | add docs for V1 tokenization and Compilation #67

Merged
merged 2 commits into from
Oct 4, 2021

Conversation

jpmarcotte
Copy link
Contributor

No description provided.

@jpmarcotte jpmarcotte self-assigned this Sep 7, 2021
Copy link
Contributor

@mucha55 mucha55 left a comment

Choose a reason for hiding this comment

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

Hahaha, that's kind of a mess

|Regex|`namespace(\s+)Neighborhoods\\BuphaloTemplateTree\\PrimaryActorName`|`namespace **NAMESPACE_PREFIX_TOKEN****NAMESPACE_RELATIVE_TOKEN**\**PRIMARY_ACTOR_SHORT_PASCAL_CASE_NAME_TOKEN**`|
|Regex|`namespace(\s+)Neighborhoods\\BuphaloTemplateTree`|`namespace **NAMESPACE_PREFIX_TOKEN****NAMESPACE_RELATIVE_TOKEN**`|
|Regex|`use(\s+)Neighborhoods\\BuphaloTemplateTree\\PrimaryActorName`|`use **NAMESPACE_PREFIX_TOKEN****NAMESPACE_RELATIVE_TOKEN**\**PRIMARY_ACTOR_SHORT_PASCAL_CASE_NAME_TOKEN**`|
|Regex|`use(\s+)Neighborhoods\\BuphaloTemplateTree/`|`use **NAMESPACE_PREFIX_TOKEN**`|

Choose a reason for hiding this comment

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

is the trailing slash here intentional?

Choose a reason for hiding this comment

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

ah looks like you fixed it in a subsequent PR

Comment on lines +64 to +65
|String|`PrimaryActorNameInterface`|`**PRIMARY_ACTOR_SHORT_PASCAL_CASE_NAME_TOKEN**Interface`|
|String|`{Template File Name}Interface`|`**SHORT_PASCAL_CASE_NAME_TOKEN**Interface`|

Choose a reason for hiding this comment

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

It would seem like these can be "combined" with the versions that have preceding \s (or really those earlier rows can be omitted?)
but if there's an implied ^ or \s in front of each of these, then nevermind
or there may be some other reason these should be separate, e.g. this is a literal catalog of what the code does and the code treats these separately, despite the overlap. thus a reason for a v2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a literal catalog of what the code does and the code treats these separately, despite the overlap. thus a reason for a v2.

☝🏻

Yup. The current code is a mess and a lot of it could probably be simplified in ways that would not result in any functional changes.

@jpmarcotte jpmarcotte merged commit a7b2d12 into master Oct 4, 2021
@jpmarcotte jpmarcotte deleted the BUPH-117-document-v1-tokenization-compilation branch October 4, 2021 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants