search

metafacture / metafacture-core

Core package of the Metafacture tool suite for metadata processing.
https://metafacture.org
Apache License 2.0
71 stars 34 forks source link

Metamorph tests verify that no unexpected interactions occurred (second and final batch). #413

Closed blackwinter closed 2 years ago

blackwinter commented 2 years ago

Continuation of #341.

Resolves #339.

blackwinter commented 2 years ago

This should be rather non-controversial. Can I merge?

dr0i commented 2 years ago

I had an eye on this PR but it wasn't assigned anyone. Let me have a short glimpse and will give you ok.

dr0i commented 2 years ago

Is there some file which defines and checks these style (indentations etc.) ?

blackwinter commented 2 years ago

Is there some file which defines and checks these style (indentations etc.) ?

No. Checkstyle is disabled for tests.

dr0i commented 2 years ago

Why is this disabled - I imagine that new code could then be committed without being "properly" styled ?

blackwinter commented 2 years ago

Why is this disabled

Because we've got too much old code that doesn't conform. It would be a major undertaking to get a clean build. (Maybe we could work with suppressions, though?)

dr0i commented 2 years ago

I want to emphasize that I very much like the idea to have all committers use the same formatting style ... atm I only reformat the sections I work on, not the whole file, because that would blur to much the important changes . With your changes re style I still cannot use the "format file on save" because it would sometimes reformat "too much".

Also, do you have an idea how to use '.editorconfig' or whatever to impose alphabetically ordering for imports? (I guess some other styles are also missing a programmatically usable definition).

dr0i commented 2 years ago

It would be a major undertaking to get a clean build

I wouldn't mind to do that, even if it means to open and save 500 files :) Question remains: is it even possible to impose that via an config file?

blackwinter commented 2 years ago

I don't think we have a formatting convention specified anywhere. Checkstyle is mostly concerned with code style and EditorConfig only allows to enforce the most basic standards.

If you're using automatic formatting that doesn't conform with everyone else's - automatic or manual - formatting then we're certainly going to have a lot of churn.

Also, do you have an idea how to use '.editorconfig' or whatever to impose alphabetically ordering for imports?

We've got a Checkstyle check for that (see 00533c5).

Question remains: is it even possible to impose that via an config file?

We'd have to look into code formatters, I suppose. (Spotless may be an option.)

blackwinter commented 2 years ago

I'm just a little puzzled why this pull request triggered this formatting discussion ;) It doesn't really deal with formatting changes, does it?

dr0i commented 2 years ago

Some files just changed their indendation. However, I agree that this is not the place to discuss the issue. +1

blackwinter commented 2 years ago

Some files just changed their indendation.

Now I see what you mean. Yeah, there were some files with incorrect indentation. After the actual (functional) changes only two or three whitespace-only diffs remained, so I didn't really notice.

Anyway, thanks for reviewing.