Skip to content
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

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

Merged
merged 7 commits into from
Nov 9, 2021

Conversation

blackwinter
Copy link
Member

Continuation of #341.

Resolves #339.

@blackwinter blackwinter marked this pull request as ready for review October 30, 2021 18:50
@blackwinter
Copy link
Member Author

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

@dr0i
Copy link
Member

dr0i commented Nov 9, 2021

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
Copy link
Member

dr0i commented Nov 9, 2021

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

@blackwinter
Copy link
Member Author

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

No. Checkstyle is disabled for tests.

@dr0i
Copy link
Member

dr0i commented Nov 9, 2021

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

@blackwinter
Copy link
Member Author

blackwinter commented Nov 9, 2021

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
Copy link
Member

dr0i commented Nov 9, 2021

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
Copy link
Member

dr0i commented Nov 9, 2021

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

dr0i commented Nov 9, 2021

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

@blackwinter
Copy link
Member Author

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.

@blackwinter blackwinter merged commit 49408cb into master Nov 9, 2021
@blackwinter blackwinter deleted the 339-metamorphVerifyNoMoreInteractionsPart2 branch November 9, 2021 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metamorph tests should verify that no unexpected interactions occurred
2 participants