-
Notifications
You must be signed in to change notification settings - Fork 3
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
Created flat tokens as additional output #53
Conversation
I would say it is fine to look over the code, especially if you find major architectural improvement points it is nice to know that now. Otherwise I would say that the flat tokens ( |
I would say the rendering into tokens code is pretty much done. It now supports all exposed types from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After getting more acquainted with your code, I've got the following high-level design question that I would like to understand before I dive deeper into the code.
That sounds like exactly what the new
I plan on replacing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good on a high level I think! Good job! So now let's move down one level of detail, which means looking at the public API of public_items. Let's do a diff (The feeling of meta when doing this never gets tiresome for me):
Click to expand `cargo public-items --diff-git-checkouts main main-douweschulte`
Removed items from the public API
=================================
pub fn public_items::PublicItem::ne(&self, other: &PublicItem) -> bool
Changed items in the public API
===============================
pub fn public_items::PublicItem::cmp(&self, other: &PublicItem) -> $crate::cmp::Ordering
pub fn public_items::PublicItem::cmp(&self, other: &Self) -> std::cmp::Ordering
pub fn public_items::PublicItem::eq(&self, other: &PublicItem) -> bool
pub fn public_items::PublicItem::eq(&self, other: &Self) -> bool
pub fn public_items::PublicItem::partial_cmp(&self, other: &PublicItem) -> $crate::option::Option<$crate::cmp::Ordering>
pub fn public_items::PublicItem::partial_cmp(&self, other: &Self) -> Option<std::cmp::Ordering>
Added items to the public API
=============================
pub enum public_items::tokens::ChangedTokenStream
pub enum public_items::tokens::Token
pub enum variant public_items::tokens::ChangedTokenStream::Changed
pub enum variant public_items::tokens::ChangedTokenStream::Same(TokenStream)
pub enum variant public_items::tokens::Token::Function(String)
pub enum variant public_items::tokens::Token::Generic(String)
pub enum variant public_items::tokens::Token::Identifier(String)
pub enum variant public_items::tokens::Token::Keyword(String)
pub enum variant public_items::tokens::Token::Kind(String)
pub enum variant public_items::tokens::Token::Lifetime(String)
pub enum variant public_items::tokens::Token::Primitive(String)
pub enum variant public_items::tokens::Token::Qualifier(String)
pub enum variant public_items::tokens::Token::Self_(String)
pub enum variant public_items::tokens::Token::Symbol(String)
pub enum variant public_items::tokens::Token::Type(String)
pub enum variant public_items::tokens::Token::Whitespace
pub fn public_items::diff::ChangedPublicItem::changed_tokens(&self) -> Vec<ChangedTokenStream>
pub fn public_items::diff::ChangedPublicItem::clone(&self) -> ChangedPublicItem
pub fn public_items::diff::PublicItemsDiff::clone(&self) -> PublicItemsDiff
pub fn public_items::tokens::ChangedTokenStream::clone(&self) -> ChangedTokenStream
pub fn public_items::tokens::ChangedTokenStream::eq(&self, other: &ChangedTokenStream) -> bool
pub fn public_items::tokens::ChangedTokenStream::fmt(&self, f: &mut $crate::fmt::Formatter<'_>) -> $crate::fmt::Result
pub fn public_items::tokens::ChangedTokenStream::ne(&self, other: &ChangedTokenStream) -> bool
pub fn public_items::tokens::Token::clone(&self) -> Token
pub fn public_items::tokens::Token::eq(&self, other: &Token) -> bool
pub fn public_items::tokens::Token::fmt(&self, f: &mut $crate::fmt::Formatter<'_>) -> $crate::fmt::Result
pub fn public_items::tokens::Token::function(text: impl Into<String>) -> Self
pub fn public_items::tokens::Token::generic(text: impl Into<String>) -> Self
pub fn public_items::tokens::Token::identifier(text: impl Into<String>) -> Self
pub fn public_items::tokens::Token::keyword(text: impl Into<String>) -> Self
pub fn public_items::tokens::Token::kind(text: impl Into<String>) -> Self
pub fn public_items::tokens::Token::len(&self) -> usize
pub fn public_items::tokens::Token::lifetime(text: impl Into<String>) -> Self
pub fn public_items::tokens::Token::ne(&self, other: &Token) -> bool
pub fn public_items::tokens::Token::primitive(text: impl Into<String>) -> Self
pub fn public_items::tokens::Token::qualifier(text: impl Into<String>) -> Self
pub fn public_items::tokens::Token::self_(text: impl Into<String>) -> Self
pub fn public_items::tokens::Token::symbol(text: impl Into<String>) -> Self
pub fn public_items::tokens::Token::text(&self) -> &str
pub fn public_items::tokens::Token::type_(text: impl Into<String>) -> Self
pub fn public_items::tokens::TokenStream::clone(&self) -> TokenStream
pub fn public_items::tokens::TokenStream::default() -> TokenStream
pub fn public_items::tokens::TokenStream::eq(&self, other: &TokenStream) -> bool
pub fn public_items::tokens::TokenStream::extend(&mut self, tokens: impl Into<Self>)
pub fn public_items::tokens::TokenStream::fmt(&self, f: &mut $crate::fmt::Formatter<'_>) -> $crate::fmt::Result
pub fn public_items::tokens::TokenStream::fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result
pub fn public_items::tokens::TokenStream::from(token: Token) -> Self
pub fn public_items::tokens::TokenStream::from(tokens: &[Token]) -> Self
pub fn public_items::tokens::TokenStream::from(tokens: Vec<Token>) -> Self
pub fn public_items::tokens::TokenStream::is_empty(&self) -> bool
pub fn public_items::tokens::TokenStream::len(&self) -> usize
pub fn public_items::tokens::TokenStream::ne(&self, other: &TokenStream) -> bool
pub fn public_items::tokens::TokenStream::push(&mut self, token: Token)
pub fn public_items::tokens::TokenStream::remove_from_back(&mut self, len: usize)
pub fn public_items::tokens::TokenStream::tokens(&self) -> impl Iterator<Item = &Token> + '_
pub fn public_items::tokens::TokenStream::tokens_len(&self) -> usize
pub macro public_items::ws!
pub mod public_items::tokens
pub struct public_items::tokens::TokenStream
pub struct field public_items::PublicItem::tokens: tokens::TokenStream
pub struct field public_items::tokens::ChangedTokenStream::Changed::inserted: TokenStream
pub struct field public_items::tokens::ChangedTokenStream::Changed::removed: TokenStream
pub struct field public_items::tokens::TokenStream::tokens: Vec<Token>
Disregarding the parts of the API that is related to diffing (see separate comment about that), one high-level comment I have is that I think it would be good if we could remove all API that allows clients to create tokens themselves. Having such an API vastly complicates upholding backwards compatibility. I think it is good to be as conservative as possible when it comes to the public API, because one of the biggest problems we can get long-term is to get locked in to a public API that we are not happy with because it prevents us from improving the library further.
This is obviously just my thoughts, and I am very open to being convinced otherwise. But I don't really see a need for clients to create tokens themselves. But you do, perhaps?
Yes I do agree that we need to remove all token creation APII, I will scrutinize the API diff and remove all code related to that. |
I Note the missing cargo public-items --diff-git-checkouts upstream/main main
Besides that I created a single commit which removes all code related to token diffing. I am not really sure how well git will handle stuff like this if now both PRs are merged. If you know a better way to handle this please let me know. |
All tests now pass except for the two that take in the output from a different file. These fail for some reason while the diff between both outputs (according to the test helper) is nothing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't be able to go through it all at once, so here are a couple of early comments. I plan on leaving more comments later as I go along doing the review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think tests are failing a bit too much for me to do a very detailed review. Do you think you could make all tests pass before the next review round? Let me know if you think that would be too much work. Again, no stress, it is fine if it takes you a while.
I feel like maybe we can simplify render.rs a bit? But before all tests pass there is no point in trying to figure out how, because before tests pass we do not know under what constraints we can refactor.
Co-authored-by: Martin Nordholts <enselic@gmail.com>
I thought I got all tests to pass, but I missed all the nice ones. Because two were randomly crashing in Points to go over:
And I do agree to keep the sort order out of this PR otherwise the output from the tests is just unusable. |
I'm not a fan of that to be honest, because it will make diffing more noisy. In general I like diffs to be on an item-level. Ideally also when doing simple text-based diffs. I am open to changing that later, but if it is OK with you I would like to not do it in this PR. Also of importance:
Our "gold standard" is Apart from the above it looks like it's only generics that indeed needs some tweaking. Great work so far! Looking forward to your next update! (I also changed repo settings now so that CI should trigger for you automatically. There's also |
@@ -1,2 +1,2 @@ | |||
pub mod thiserror | |||
pub proc macro thiserror::Error! | |||
pub proc macro thiserror::#[derive(Error)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I created a conflict with you here with #68
I try to not create conflicts but forgot to double-check this one
Fortunately it should be easy for you to resolve; change from
pub proc macro comprehensive_api_proc_macro::SimpleDeriveMacro!
to
pub proc macro comprehensive_api_proc_macro::#[derive(SimpleDeriveMacro)]
in ./tests/expected_output/comprehensive_api_proc_macro.txt
Let me know if you would like me try to push a commit to this PR that resolves the conflict.
(The reason I work towards removing all .json from the repo is that that makes it easier to rename the lib to public-api
later.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The reason I work towards removing all .json from the repo is that that makes it easier to rename the lib to public-api later.)
Just a note (also mentioned in a separate comment): I now feel done with a big restructuring on how tests are run. Mainly stopping to use public_items itself for tests, because that becomes a real pain when we will rename the lib to public-api. The PR is at #70, but I will not merge it before this PR is merged, to avoid causing more troubles for you. Again, I am very open to resolving the thiserror-1.0.30.txt-conflict for you, if you wish. It is not fair for you to solve problems caused by me :)
tests/bin_tests.rs
Outdated
@@ -6,7 +6,27 @@ fn print_public_items() { | |||
let mut cmd = Command::cargo_bin("public_items").unwrap(); | |||
cmd.arg("./tests/rustdoc_json/public_items-v0.4.0.json"); | |||
cmd.assert() | |||
.stdout(include_str!("./expected_output/public_items-v0.4.0.txt")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a bit of repetition here: It would be great if you could avoiding changing "expected output" (here for example). Not only to avoid conflicts with #70 (which I will wait with merging until this PR is merged though), but also because good practice in and of itself, when doing big changes.
Edit: I am fine with changing Error!
to #[derive(Error)]
though, because that is such a good change.
@@ -1,2 +1,2 @@ | |||
pub mod thiserror | |||
pub proc macro thiserror::Error! | |||
pub proc macro thiserror::#[derive(Error)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(The reason I work towards removing all .json from the repo is that that makes it easier to rename the lib to public-api later.)
Just a note (also mentioned in a separate comment): I now feel done with a big restructuring on how tests are run. Mainly stopping to use public_items itself for tests, because that becomes a real pain when we will rename the lib to public-api. The PR is at #70, but I will not merge it before this PR is merged, to avoid causing more troubles for you. Again, I am very open to resolving the thiserror-1.0.30.txt-conflict for you, if you wish. It is not fair for you to solve problems caused by me :)
@douweschulte Hi! I developed a commit on top of this PR that makes all tests pass and that takes care of my own code review comments. Is it OK if I push it to this PR and then merge this PR? We can then of course iterate on the code further, but it is much nicer to do that in small increments in small PRs rather than have one big PR open :) (Note that the commit does a "revert" of changes to expected output. To diff expected output, it is better to diff against v0.8.0 than towards the parent of my commit.) I hope I don't come across as "stealing" your work. Or cause other problems for you. Just trying to be helpful 🙂 This was a great way for me to get some hands on experience with your new code though. Please let me know if you think I am doing anything wrong here! If you want to take my code through some code review rounds that is perfectly fine of course. And if you want me to merge my code with in-progress code you have locally, that is also perfectly fine. Or discard my code entirely if you were almost done locally. Just let me know whatever your preferences are. |
Nice work! I do agree that small PRs are better here once this beast has been merged. I think it would be fine to merge and gather up all small leads in separate PRs. I was planning on doing this work this weekend but the fact that you build it is a pleasant surprise. This huge beast of a PR was getting a bit hard to fully understand every detail of all the time. And I got a surprise load of work to do in other projects this week. So I would propose to merge this, I will then gather all loose ends from my side and open issues/PRs respectively to continue our discussion there. As I said I got some work to do on other projects as well so I maybe will be a bit less active for a while, but I want to continue work here as well. |
@douweschulte Great! I will merge this PR then, and shortly after I will also merge #70. Stay tuned :) |
Here I created a flat simple token based output, this can be used for syntax colouring and better change diffs. In some cases the output is not very nice yet, especially generics. And sometimes the ID comes peeking out, in structs in enum variants. The code organisation/API is of course open for discussion.