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

Also mark contexts referenced by name as "no prototype" (same as ST) #180

Merged
merged 1 commit into from
Jun 29, 2018

Conversation

robinst
Copy link
Collaborator

@robinst robinst commented Jun 28, 2018

Noticed this while working on changing things to an arena. This doesn't
fix any syntests, but mirrors what ST does.

It affects the C# syntax. Here's a tiny syncat example that changed; before:

screen shot 2018-06-28 at 11 06 02

After (same as ST):

screen shot 2018-06-28 at 11 06 10

"<source.test>",
"<source.test>, <test.good>",
"<source.test>",
], "Expected test.bad to not match");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The existing assertion functions here make it a bit hard to test that a scope didn't match. With this, it's nice and explicit.

As a future change, it would be nice to move the syntest functionality so that we can use it in tests. That would make it easier to compare against ST.

Another note: Should we start moving higher level parsing tests out of this file, as it's getting quite big? They could live in files in the tests directory, as I think they're only using public API anyway.

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 okay with moving it to another file if you'd like, although I don't mind big files myself. Also agree on putting the syntest stuff in the library eventually for use by tests.

@robinst
Copy link
Collaborator Author

robinst commented Jun 28, 2018

Build failed because of base64 used by plist. Opened a PR to upgrade plist which might solve that: #181

@robinst
Copy link
Collaborator Author

robinst commented Jun 28, 2018

So the build failure is a known problem on beta: marshallpierce/rust-base64#64

Should we stop running cargo doc in the beta build for now?

@keith-hall
Copy link
Collaborator

I suspect that this fixes #160

@robinst
Copy link
Collaborator Author

robinst commented Jun 28, 2018

I suspect that this fixes #160

Just tried, it doesn't (unless I tried wrong).

@keith-hall
Copy link
Collaborator

Doh, just wishful thinking on my part then; thanks for checking. (I doubt you tried wrong ;))

@robinst robinst mentioned this pull request Jun 28, 2018
9 tasks
@robinst
Copy link
Collaborator Author

robinst commented Jun 28, 2018

Was hoping too :). I still think our with_prototype implementation is different somehow, but I haven't had a chance to sit down and understand it yet.

Copy link
Owner

@trishume trishume left a comment

Choose a reason for hiding this comment

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

Huh interesting, before reading this I think I speculated in your other PR that Sublime actually didn't do this because doing this is weird. Thanks for figuring this out.

"<source.test>",
"<source.test>, <test.good>",
"<source.test>",
], "Expected test.bad to not match");
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 okay with moving it to another file if you'd like, although I don't mind big files myself. Also agree on putting the syntest stuff in the library eventually for use by tests.

@trishume
Copy link
Owner

And yah if you can figure out how to make Travis not run the docs on beta that would be good.

Noticed this while working on changing things to an arena. This doesn't
fix any syntests, but mirrors what ST does.
@robinst robinst force-pushed the no-prototype-for-context-referenced-by-name branch from d034768 to 0048072 Compare June 29, 2018 04:44
@robinst
Copy link
Collaborator Author

robinst commented Jun 29, 2018

Fixed in #184 and merged it, hope you don't mind. Rebased this and other PRs.

@trishume trishume merged commit 312a648 into master Jun 29, 2018
@trishume trishume deleted the no-prototype-for-context-referenced-by-name branch June 29, 2018 15:17
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.

None yet

3 participants