gnodet commented on pull request #391:
URL: https://github.com/apache/maven/pull/391#issuecomment-735455117
@rfscholte overall, because of the number of commits, it's really difficult
to comment on the diffs, maybe at this point, a squashed commit would be easier
to review, since there are so many commits... Hence I'm raising a few points
here rather than in possible diff comments...
### File vs Raw vs Effective model
The concept behind the raw model seems to change quite a bit. I've watched
the video, so I roughly get the idea. However, there are a few spots where
things are unclear : for example the `buildRawModel()` method from the
`DefaultModelBuilder` seems to return... a file model. It would be nice to
have it documented somewhere.
### FileModelSource vs ArtifactModelSource
Could you please clarify the usage of `FileModelSource` ? There are a few
spots where `modelSource instanceof FileModelSource` is used. It seems that
`FileModelSource` is used to denote a pom file which is being built (i.e. part
of the reactor) whereas `ArtifactModelSource` denotes a pom file coming from a
repository. What is unclear is why would is why the process would be specific
to the Source implementation... If pom files should be read differently,
shouldn't that be an explicit part of the `BuildModelRequest` instead of
implicit from the type of the Source ?
### GA instead of GAV
All the model read seem to be indexed by group/artifact and not by
group/artifact/version. Does that mean that a single reactor can not used 2
different dependencies with different versions ? I know this can't happen for a
single model, but this is currently supported for two different modules. If
the reactor only contains a single model for a given GA, how will that work ?
### SAX vs Stax
Another point is the usage of SAX rather than Stax API (honestly, I have
hardly seen anybody using SAX since years). The models are read using a pull
parser, so I'm not sure why introducing a push SAX filter to convert the file
into a stream to be consumed by the pull parser in another thread. This seems
really inefficient. In huge builds, a few hundreds or more pom files can be
read, so that could really be a real problem. I wonder if the
`ModelSourceTransformer` would be better redesigned as a pull filter somehow.
Maybe something like
```
XmlPullParser transform( XmlPullParser parser, TransformerContext context
)
throws IOException, TransformerException;
```
I know there are a bunch of things written in `maven-xml` already but this
decision can have a big impact. An alternative would be to only use a SAX
parser, in which case the xml -> model mapping would have to be rewritten. I
don't really care, but it does not seem a very good option for the future mix
push/pull parsing together to read a model.
### ModelReader
In the `DefaultModelReader`, only the `Model read( File input, Map<String,
?> options )` method does use the `ModelSourceTransformer` to read the Model.
If there's a good reason for not performing a transformation when using a
`Reader` or an `InputStream`, it should be written in the javadoc, but it seems
to me it can be extended to the other methods as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
