lib: remove Syntex dependency #26

hcpl · 2017-10-23T13:11:48Z

It's a big dependency, slow to compile and not maintained anymore.
Moving to syn.

There is a caveat though: version-sync relied on span information
gathered from syntex to provide more helpful messages. syn 0.11.11
doesn't have such a functionality, though there are plans to incorporate
recent changes in procedural macro system to enable spans there too.
Current syn master contains such code, but is unstable.

It's a big dependency, slow to compile and not maintained anymore. Moving to `syn`. There is a caveat though: `version-sync` relied on span information gathered from `syntex` to provide more helpful messages. `syn` 0.11.11 doesn't have such a functionality, though there are plans to incorporate recent changes in procedural macro system to enable spans there too. Current `syn` master contains such code, but is unstable.

hcpl · 2017-10-23T13:18:25Z

I have a question regarding the code that used syntex: did you intend to run checks for all #[doc(html_root_url = "...")] found in the source, regardless of how many #[doc(...)] attributes or how many html_root_url = "..." have already been found? The code I'm PR`ing only hits #[doc(html_root_url = "...")] once, then returns immediately for both successful and failed checks.

mgeisler · 2017-10-25T19:06:14Z

.gitignore

@@ -1,3 +1,6 @@
 /target/
 Cargo.lock
 *~
+**/*.rs.bk


A simple *.rs.bk should be enough -- like the *~ pattern above.

Yeah, you're right.

Well, that's just a pattern I took from .gitignore autogenerated by cargo new foo so I didn't give it a second thought.

mgeisler · 2017-10-25T19:08:39Z

.gitignore

@@ -1,3 +1,6 @@
 /target/
 Cargo.lock
 *~
+**/*.rs.bk
+[._]*.sw?
+[._]sw?


I'm curious, what kind of files are these? Vim swap files? What is the initial + about?

*.rs.bk - rustfmt backup files, in case if you want to diff the original and the result.

.*.sw? - vim swap files that match .*.swp, .*.swo, .*.swn etc. (the last letter runs backwards) and _*.sw? for its Windows counterpart.

.sw? - vim swap files for directories (just .swp, .swo etc., no filename part), also matches those for Windows.

And + is just "added lines" from git diff :D

Great, thanks for the info!

Actually, the (presumably complete) set of possible swap file names is even more complex: https://unix.stackexchange.com/a/326737. I just don't bother with this in my projects, though I don't know if that can be applied to yours'.

mgeisler · 2017-10-25T19:14:21Z

src/lib.rs

+
+                    match check_result {
+                        Ok(()) => {
+                            // FIXME


Will you expand this comment to say that we should add line numbers again when the functionality is there?

Good idea, I'll do it.

mgeisler · 2017-10-25T19:20:20Z

Hey @hcpl, thanks for another well-made patch!

I actually started with the syn crate but then I figured it was nice to have better error messages. The dependencies are only compiled once, so does it matter much? Back then, I tried to collect the different parsing options on the users forum.

However, since there are plans for adding span information to syn, I gues it would be okay to switch to that crate instead — I don't like to depend on abandoned code :-) Do you have any idea of when support for spans could land in stable?

Would it be possible (with a reasonable amount of code) to have the line number information when version-sync is used with nightly Rust? Only do this if it doesn't make the whole thing super ugly with lots of conditional pieces of code.

mgeisler · 2017-10-25T19:27:24Z

I have a question regarding the code that used syntex: did you intend to run checks for all #[doc(html_root_url = "...")] found in the source, regardless of how many #[doc(...)] attributes or how many html_root_url = "..." have already been found?

Oh, good question... I did not consider that :-)

I guess it would be reasonable to run through all #[doc(...)] attributes and report an error if any of them defines a html_root_url that doesn't match the expected URL. So the current code probably returns too soon.

I don't actually know what will happen if you have multiple html_root_url attributes defined -- maybe the last one wins? We could raise an error too if we see more than one html_root_url, but I kind of don't see linting the attributes as the main purpose of the tool. However, I wouldn't mind either way since we also raise an error if we cannot parse a code block marked as TOML code.

hcpl · 2017-10-25T21:13:59Z

@mgeisler for me personally, there is also a matter of supporting code with newest features - it is much more pronounced for new nightly features because there won't ever be code in an abandoned parser that can deal with them in foreseeable future.

syn does support (some) nightly Rust code while working on stable like const fn or specializations through default impl methods. Though you could argue that syntex is likely to support even more due to its origin... I can only say that #[doc(html_root_url)] is something sane people only write once for the whole crate and at the very top of src/lib.rs, so that we could apply syn only to a part of the whole code and after that we could hack this somehow, e.g. traversing lines & regex-matching them (I definitely don't like dirty-hacking like this but IMO it does produce reasonable results in practice, so I'll leave this to your judgement). The syntax of attributes being relatively stable helps.

Since the only code in version-sync that touches Rust source files is check_html_root_url function, there will only be two conditional pieces. For now, I don't know if there will be more Rust code-parsing dependent features in this crate. But if that ever happens, we can localize every such feature under a module and reexport them to root afterwards.

Ah, didn't know that usesr forum thread existed at all :D. Like I said before, syn does parse nightly code to some extent but if one really needs this working with cutting edge nightly, then we'll have to go nightly all the way and gate it properly. syntex won't be able to help us in this case anyway.

mgeisler · 2017-10-25T21:29:06Z

Okay, last question: can you update the README to match the new output? It has an example of a html_root_url attribute being flagged and shows the line number in the error :-)

mgeisler · 2017-10-25T21:34:59Z

I can only say that #[doc(html_root_url)] is something sane people only write once for the whole crate and at the very top of src/lib.rs,

Completely agreed! I'm happy if version-sync handles the normal cases correctly, e.g., the case where a sane developer forgets to update the html_root_url attribute.

so that we could apply syn only to a part of the whole code and after that we could hack this somehow, e.g. traversing lines & regex-matching them (I definitely don't like dirty-hacking like this but IMO it does produce reasonable results in practice, so I'll leave this to your judgement).

It's funny you should mention it... I was actually experimenting with that idea back in the day when I used syn to parse the files :-) It seems like an okay idea and it should work for 99% of the cases where the URL is simply written as a simple literal in the source code.

It breaks down if the URL contains escaped characters in the source (which escape did the user use?) but I don't think we need to care about that. The usual normal case is enough and later we'll get proper span information from syn.

hcpl · 2017-10-30T21:45:44Z

@mgeisler hope I got it this right!

Regarding temporary solution for line reporting: should I finish it off in this PR or let it be done afterwards? Also there is an option of not doing anything after this PR (if it gets merged now) just because it may not be worth it after all.

mgeisler · 2017-10-31T17:53:58Z

@hcpl: #27 was just opened and that would make this PR unnecessary. I don't know which one we should merge now :-)

hcpl · 2017-10-31T19:04:41Z

@mgeisler that's a very long-awaited docs.rs feature for me! Sure I'd propose to close this PR, but there are still complications addressed in #27, so I'd like to postpone any action before we resolve them.

mgeisler · 2017-11-01T06:48:12Z

@hcpl Yeah, I am not 100% convinced that the functionality needs to go.

mgeisler · 2017-11-01T19:05:16Z

I've merged this for now. It looks like we should keep this functionality around for a little bit longer, at least until rustdoc links to docs.rs by default (rust-lang/rust#42301). Thanks for the PR!

hcpl · 2018-01-08T19:03:26Z

@mgeisler, syn 0.12 and quote 0.4 were just released -- https://users.rust-lang.org/t/syn-0-12-complete-redesign-for-all-your-macros-1-1-and-2-0-needs/14917 -- we can have span information now!

mgeisler · 2018-01-08T22:43:09Z

Thanks for the heads up, @hcpl ! That's really cool to hear. I'll make a note of updating the dependency, but I don't think I'll get to it in the next few weeks. Feel free to beat me to it :-)

mgeisler reviewed Oct 25, 2017

View reviewed changes

lib: expand the comment about re-adding span info

29b1c7f

readme: edit to match the new output

c251335

mgeisler merged commit 9c15ed0 into mgeisler:master Nov 1, 2017

mgeisler mentioned this pull request Nov 1, 2017

Removed html_root_url verification logic #27

Closed

lib: remove Syntex dependency #26

lib: remove Syntex dependency #26

Uh oh!

Conversation

hcpl commented Oct 23, 2017

Uh oh!

hcpl commented Oct 23, 2017

Uh oh!

mgeisler Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

hcpl Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

hcpl Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgeisler Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

hcpl Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgeisler Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

hcpl Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

mgeisler Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

hcpl Oct 25, 2017

Choose a reason for hiding this comment

Uh oh!

mgeisler commented Oct 25, 2017

Uh oh!

mgeisler commented Oct 25, 2017

Uh oh!

hcpl commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgeisler commented Oct 25, 2017

Uh oh!

mgeisler commented Oct 25, 2017

Uh oh!

hcpl commented Oct 30, 2017

Uh oh!

mgeisler commented Oct 31, 2017

Uh oh!

hcpl commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgeisler commented Nov 1, 2017

Uh oh!

mgeisler commented Nov 1, 2017

Uh oh!

hcpl commented Jan 8, 2018

Uh oh!

mgeisler commented Jan 8, 2018

Uh oh!

Uh oh!

hcpl Oct 25, 2017 •

edited

Loading

hcpl Oct 25, 2017 •

edited

Loading

hcpl commented Oct 25, 2017 •

edited

Loading

hcpl commented Oct 31, 2017 •

edited

Loading