-
Notifications
You must be signed in to change notification settings - Fork 12
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
API: Big API Module Migration #268
Conversation
The changes sound reasonable. I would go further and maybe think of making the public module structure more flat. For example reduce the There could be an explicit recommendation about not importing Modules make it harder to use outside, and reexports should be used for that, unless there would be name conflicts and/or the module name bears an important part of the context for the type name. Both are the case for We could still have mod ty;
mod item;
mod export;
// ...
pub use ty::*;
pub use item::*;
pub use expr::*;
// ... Same thing about Btw. So in general LGTM |
error[E0603]: module `expr` is private | ||
--> $DIR/diag_msg_uppercase_start.rs:3:23 | ||
| | ||
3 | use marker_api::{ast::expr::ExprKind, context::MarkerContext}; |
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 doesn't look right as it complains about the module being private
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 new ui_test version blesses tests by default. That's why I didn't see the fail. I don't like that, I'll make it error by default. There is a setting for that. (Will do that later)
Thank you!
Having everything in the root of The main drawback I see, is the missing structure in the documentation. After this change, it's harder to find something in the I'm probably also a bit biased towards the pub module structure because that's just how the API was until now. I feel like the advantages and disadvantages are almost even. The latest commits now have the "flatter" module structure. I'm leaning towards this one, since you recommended it. But I'll still think a bit more about it, before making a final decision (tomorrow). @xFrednet FIXME: Cleanup the namespace mess in the driver (I just fixed all the imports. Now, the used names in the driver are a mess. Sometimes they use the module |
587320b
to
e42509b
Compare
I've rebased, to get the
|
Summary
marker_api::sem
modulemarker_api::common
moduleSem
andSyn
prefix from types and genericsmarker_api::prelude
no longer contains the semantic and syntacticTyKind
enumsmarker_api::prelude
now imports thesem
andast
namesmarker_api::ast
module has been flattenedmarker_api::lint
andmarker_api::interface
are now privateMotivation
Moving modules
The
ast
module ofmarker_api
contained several items, which were not part of the syntactic representation. The semantic type representation is an example of something which didn't quite belong in the AST module.Rustc also has this separation with
rustc_middle
providing the semantic type representation andrustc_hir
holding the HIR representation, with an AST structure and spans etc. (It's more complicated than this, but you get the idea)This is one of the biggest breaking changes I've done in Marker so far. It's better to make this separation now, while there are very very few to no users.
Removing the
Sem
andSyn
prefixThis is a change I'm actually uncertain about. The
Sem
andSyn
prefixes were previously used to distinguish between the semantic and syntactic variants of types. For example,SynTyKind
andSemTyKind
. Now that they live in different modules, they can have the same name.I believe that common coding conventions dictate, that the
Syn
andSem
prefix should be removed, as it's just a repetition of the module, and they make the name more verbose than it needs to be.The reason, why I'm unsure about this change, is that I liked the name-based distinction. When reviewing code and reading
SemTyKind
it was 100% clear which kind of type this is. Now a user will probably import theTyKind
. When reading something like thisTyKind::Ref(x) = expr.ty()
, it requires more context to know if this is a semantic or syntactic type. Users can still specify the path, like thissem::ty::TyKind
but this is more verbose, than the previousSemTyKind
.Another advantage was, that the documentation only had one entry for every type. Searching for
SemRefTy
provided one result, whileRefTy
will from now on provide two, one from the semantics and one from the AST module.It would be good to know, how often code mixes the two different
TyKind
s. That might give a good indicator if the types should have the prefixes or not. In Clippy, we rarely mix the types in one file. There are not that many lints, which require information from both kinds. Semantic types are often not even stored, but only passed to the next function to check if they belong to a specific type or implement a trait.@Veetaha If you have the time, would you mind reading the PR description, and provide feedback if this sounds reasonable?
I doubt that reviewing the actual code changes is worth it, I only moved and renamed a lot of things. The only thing I added was a short doc comment in the
sem
andast
module.This should be the last PR fore v0.3.0. I didn't think we would make it this far in so little time. We can be proud of the progress 😊