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

Develop #61

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Develop #61

wants to merge 4 commits into from

Conversation

atelich139
Copy link
Collaborator

Interfaced out the switch that determined ast.Node type and appended token accordingly. Cleaned up the way Token types were declared as constants. Removed 'getWorkspace' returning a *error.

Copy link
Owner

@gundermanc gundermanc left a comment

Choose a reason for hiding this comment

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

Can you try it with VS or VS Mac and ensure it still hackily works with Colorization, Completion, and Error squiggles?

}

// QueueFileParse queues the reparse of a file. Reparse is signaled to the caller
// via a WorkspaceUpdateCallback.
func (id WorkspaceID) QueueFileParse(fileName string, reader io.Reader, versionId uintptr) *error {
if reader == nil {
Copy link
Owner

Choose a reason for hiding this comment

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

Doesn't the Go linter prefer tabs? I've been of the opinion when in Rome...

Copy link
Owner

Choose a reason for hiding this comment

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

I'm ok with switching to spaces so long as we can put the right .editorconfig or whatever is needed to keep the files consistent in the project.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

idk actually. I never changed the default settings of the builtin to Goland.

Copy link
Owner

@gundermanc gundermanc Aug 2, 2019

Choose a reason for hiding this comment

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

In Visual Studio + VS Code we can configure these settings usually with .EditorConfig files. We can probably for Goland too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea for sure. Actually, I looked a bit closer and Goland is pulling from my IDE settings repository that's configured for other languages and I think it might have gotten messed up. I'll configure it before the next commit.

Copy link
Owner

Choose a reason for hiding this comment

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

No worries, this is a low-key side project. There are no standards :)

RESOLVEDTYPE TokenType = 4
LITERAL TokenType = 5
COMMENT TokenType = 6
KEYWORD = iota
Copy link
Owner

Choose a reason for hiding this comment

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

Are these guaranteed ascending?

Note that I hard coded integer values here to ensure they match the C# side if reordered

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes they are

Copy link
Owner

Choose a reason for hiding this comment

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

Ok, it's definitely worth a comment at least to ensure they don't get messed up.

I'd actually feel a bit more comfortable leaving the int values in place because this is a binary compatibility issue and not just a source code one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, I'd agree with that, will change back. Perhaps move their declarations to the token_service with a comment as well?

"go/token"
)

type tyNode interface {
Copy link
Owner

Choose a reason for hiding this comment

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

What's the benefit of the interfaces in this particular case given that they're all different? I think they're a helpful addition when they can be used to reduce boiler plate but in this particular case they don't seem to have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not so much about reducing boiler plate as it is about reducing dependencies on the token components. I.E. in another instance where you have to do something with many tokens but specific to each one, you just have to add that type of action as an interface method.

@gundermanc
Copy link
Owner

Can you try it with VS or VS Mac and ensure it still hackily works with Colorization, Completion, and Error squiggles?

#38

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