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

Convert Token from enum to struct #256

Merged
merged 3 commits into from
Sep 30, 2018
Merged

Convert Token from enum to struct #256

merged 3 commits into from
Sep 30, 2018

Conversation

ilyapuchka
Copy link
Collaborator

Resolves #255. Strangely enough, it makes parsing slightly slower, but I still think it's a good thing to do.
I didn't want to change too much code so I left static constructors instead of cases for interface to be the same.

public let sourceMap: SourceMap

/// Returns the underlying value as an array seperated by spaces
public lazy var components: [String] = self.contents.smartSplit()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't that be private(set)?

Copy link
Contributor

Choose a reason for hiding this comment

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

jep

Copy link
Contributor

Choose a reason for hiding this comment

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

We may also want to avoid splitting into components if the type is text?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can return just one item with the whole content, but I think this should be handled on a higher level and might as well not happening at all due to the logic flow

@djbe
Copy link
Contributor

djbe commented Sep 26, 2018

Quick question: how did you measure a slowdown?

@ilyapuchka
Copy link
Collaborator Author

in the performance test, it went from 250-something to 260-something

@djbe
Copy link
Contributor

djbe commented Sep 26, 2018

The performance test is always a bit inconsistent though, you have to run it a few times to get an idea if it's a fluke, or an actual increase.

Anyway, 10ms increase isn't a problem in this case.

@djbe
Copy link
Contributor

djbe commented Sep 26, 2018

What we're not noticing now, is a potential performance benefit in other parts of Stencil, as we only measure the lexer performance.

@djbe
Copy link
Contributor

djbe commented Sep 26, 2018

Code looks GTM.

Might be worth adding documentation like I did in cb4e514, the current codebase can be quite complicated for developers to understand, and the docs should help with that.

Also, don't forget a changelog entry! 😉

@ilyapuchka
Copy link
Collaborator Author

@djbe there is not really much to document except type properties, which are self-explanatory IMO, the Kind ass well but it already had docs.

@ilyapuchka ilyapuchka merged commit d9f6a82 into master Sep 30, 2018
@ilyapuchka ilyapuchka deleted the token-as-class branch September 30, 2018 20:48
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.

3 participants