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

initial rework of syntest to be usable internally #185

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

Conversation

keith-hall
Copy link
Collaborator

this is an initial rework of the syntest example, to make it usable internally, as it would be useful for parser tests (see #180 (comment)). I think it will need some more work to properly support assertions that extend over the end of the line (#175) and spill over onto not only the next line, but also the line afterwards etc. although such a scenario may never come up in real life use.

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.

Just some architectural requests to make it fit better as a public API.

Let me know if you made any real changes to the main body of the function you moved, I didn't read it, assuming it's just a copy-paste.

use syntect::highlighting::ScopeSelectors;
use syntect::easy::{ScopeRegionIterator};
use syntect::parsing::{SyntaxSet};
use syntect::easy::{SyntaxTestFileResult, SyntaxTestOutputOptions, process_syntax_test_assertions};
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this should go in the easy module.

That's for simple wrappers for common use cases. Syntax tests are neither a wrapper around a more advanced API, nor a common use case.

They should probably go in their own syntax_tests module or something

src/easy.rs Outdated
Success(usize),
}

pub fn process_syntax_test_assertions(syntax: &SyntaxDefinition, text: &str, testtoken_start: &str, testtoken_end: Option<&str>, out_opts: &SyntaxTestOutputOptions) -> SyntaxTestFileResult {
Copy link
Owner

Choose a reason for hiding this comment

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

If this is now going to be a public API, could you add a doc comment with short descriptions of what the parameters do? Also useful for our own reference.

src/easy.rs Outdated
@@ -174,6 +174,236 @@ impl<'a> Iterator for ScopeRegionIterator<'a> {
}
}

#[derive(Clone, Copy)]
pub struct SyntaxTestOutputOptions {
Copy link
Owner

Choose a reason for hiding this comment

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

Now that this is an API, it seems a bit weird for it to just print things to stdout.

Maybe you could add a lifetime parameter to this struct and add a output: &'a io::Write member. Alternatively you could add an extra parameter for the output to the process function.

Then the function then uses writeln! instead of println! to write to, and the process function would probably need to return an io::Result. Then syntest and possibly also unit tests could just pass stdout.

@keith-hall
Copy link
Collaborator Author

I have moved it to a new syntax_tests module and added a doc comment 👍
The main function has been modified; it used to read line by line, parsing as it went - now it reads all the syntax test assertion positions/details first, then parses line by line afterwards as a separate step and compares the results.
In terms of using io::Write, great idea. I think it's a little beyond my skillset and free time at the moment, so it'd be great if someone else wouldn't mind contributing this. It will be necessary to also modify Util::debug_print_ops - perhaps it'd make sense to rename it debug_write_ops, take an io::Write as a parameter, and create a new debug_print_ops which will just pass in stdout() (or https://doc.rust-lang.org/std/io/struct.Stdout.html#method.lock (I'm not sure which is better)).

@robinst
Copy link
Collaborator

robinst commented Jul 3, 2018

Nice, thanks for picking this up :)!

In the current form, I don't think we should make it a public API. An API with the only output being some printed text doesn't seem very useful generally.

So I would do either one of these two things:

  • Change it to pub(crate)
  • Move the printing parts back to the syntest example, and change the API to return plain structs with the information about the assertions instead.

(With the latter, we could do some nice things such as formatting as HTML so you can more easily see the actual scopes, show the results in a text editor instead, etc.)

@keith-hall
Copy link
Collaborator Author

Printed text isn't the only output - the API returns the successful/failed assertion count too ;)

I would guess that changing the API to return structs with the information about the assertions, and printing them from the syntest example would mean that we would lose the "instant" nature of the results. i.e. currently, one would see in the output about failures at the top of the syntax test file without having to wait for the whole rest of the file to be evaluated. I'd be keen to keep this, if possible.
I like the idea of formatting the output, although I don't have any concrete ideas of how it could look at the moment. (Currently I just pipe the output of syntest into a file and review it in Sublime.) Maybe outputting to HTML (like synhtml), using a specified color scheme, and then showing the failures on top of that somehow?

@robinst
Copy link
Collaborator

robinst commented Jul 3, 2018

Hehe :). Is it possible to make it return an Iterator with results? Then you could just iterate over that and print as the assertions come back.

@keith-hall
Copy link
Collaborator Author

I think that returning an iterator with the results is a cool idea, it would allow a consumer of the API to fail fast when a failure occurs, if so desired. Though I guess it would likely prevent the use cases we mentioned above, whereby the (optionally colored) output could be combined with the syntax test results, as it would only iterate over those results and then the file would have to be parsed again to achieve an integrated view, which isn't ideal.
Also, it would mean all the consumers would have to track the number of failures and thus could cause code duplication when all they care about is the totals (for summary purposes).
But using io:Write instead wouldn't magically enable this either, so maybe it's not a realistic goal at the moment?

Marking the API for internal use only would solve needing to make further complicated changes at the moment. But changing the method to pub(crate) seems to prevent the syntest example from seeing it, and I get warnings about dead code. Any ideas? :)

@trishume
Copy link
Owner

trishume commented Jul 4, 2018

Okay I'll try and give the new implementation a real review then, likely this weekend. If you'd like I can probably also do the io::Write stuff after I do that.

@robinst I agree it's not the greatest thing to have as a public API but:

  • there's unfortunately no pub(examples)
  • making it be a nice proper API may be a decent amount of work.
  • I've never been too stingy with public APIs in syntect and in this case if we implement a better API it should be pretty easy to reimplement it in terms of the new API, or do a major release and remove the old version (I doubt anyone will use it)

this will allow other functionality to be developed that can use the same syntax test implementation and show the test results in a more friendly manner etc.
@trishume
Copy link
Owner

Sorry I'm so late on reviewing this, I wanted to use my limited syntect bandwidth for stuff that needed to be in the 3.0 release. This is still on my TODO list.

@keith-hall
Copy link
Collaborator Author

No worries, the work you did on the color schemes was awesome and indeed more important ;)

@Enselic
Copy link
Collaborator

Enselic commented Mar 16, 2022

@keith-hall Is this PR something we still want to try to get in?

In general I think it is undesired to add public API only for the purpose of testing, because each increase in API surface makes all subsequent bugfixes and features more complicated to do due to the mere fact that there are more things to take into account, so in general a minimal API surface is desirable.

Couldn't we simply keep the tests as mod tests that have access to internal interfaces?

I apologize in advance for any ignorance on my part.

@keith-hall
Copy link
Collaborator Author

although not mentioned in the original post, the idea behind moving the code from an example to be part of the public API is because running syntax tests isn't just useful for testing syntect itself, but also for people writing sublime-syntax files. If syntect were to be used in a text editor, for example, then users/syntax authors might expect to be able to test their sublime-syntax/syntax_test_ files directly, and it wouldn't make sense for projects depending on syntect to copy/paste or recreate that logic when it is ST compatible and thus IMO belongs in syntect.

@Enselic
Copy link
Collaborator

Enselic commented Mar 17, 2022

Thanks for the clarification, that makes sense. I will try to find the time to help review this in a not too distant future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants