Clean up / align internal structure, names, etc.

[Bukama]

The current milestone we work on is called "Cleaner Pioneer" and includes issues covering several different like infrastructure, refacotring, polish and more. Goal is to build a solid base for future work (goal is version 1.0). But in my opinion one aspect of the term "Cleaner Pioneer" is missing: The current state of the lib itselfs is a mess in terms of folders, naming. This issue will show some points which in my opinion should be cleaned up / aligned. Maybe there are even more.

Definition of annotations

Current state

Currently most annotations are declared in own interface-files, but not all (e.g. @TempDirectory is definied inside the TempDirectory class).
Repeatable annotations (here I mean the "plural" ones that hold the array, e.g. @ReportEntries) are also declared in own interface-files, regardless the fact, that they are never used as an annotation.

Proposal:

We should define a standard so all annotations the same way. Here we should go for defining the declaration inside own interface-files (e.g. @ReportEntry).

A special case may be repeatable annotations: I accidently stumbled over this article about declaration of repeatable annotations. After reading it I think this is a nice way to provide a more intuitive and cleaner API. It also makes it easier to compare that both annotation have the same @Target and @Retention. So I suggest to following the guidance in the articel and put the array holding annotations of repeatable annotations inside the corresponding one.

Location of annotations and other files

Current state

Currently annotations, their extensions and all other files (e.g. ArgumentProviders, InvocationContexts or Utils) are declared inside the org.junitpioneer.jupiter package. A special case are the files or the parameter extensions, which were moved into a subpackage for better overview.

The JUnit API is declared in org.junit.jupiter.api and subpackages of this (e.g. org.junit.jupiter.api.extension). Note: It's also declared in a own java module but for pioneer we have already decided (see #25) that we don't want to build a multi module project (at least not yet).

Proposal:

As annotations are the API of pioneer and to get somehow aligned to JUnit I suggest to move:

• all annotations to org.junitpioneer.jupiter.api or sub packages of it, e.g. params for extensions / features with a lot of annotations.
• all extension implementations and their need classes (e.g. ArgumentProviders, InvocationContexts) to org.junitpioneer.jupiter.extension
• all other files (e.g. Util) to stay in org.junitpioneer.jupiter

As for now the vintage package should stay as it is as I don't see much additional content their.
I know that moving around API classes is something critical but as of now pioneer has not reached version 1.0 yet, I think we can make thise movearound if we want to. If we don't want to do that we can still leave the annotations in org.junitpioneer.jupiter and only move the non-API class Util to another package.

Naming convention

We should think about some naming convetions, like every extension should be suffixed with Extension like ReportEntryExtension. Almost all do this at the moment, except TempDirectory. Maybe we should definie this too for ArgumentProviders, InvocationContexts and something like this.

What we should also consider are names of extension namespaces.

Here I suggest to define that the Namespace class and the extension class name should be used (following exampe is from the DefaultLocaleExtension).

import org.junit.jupiter.api.extension.ExtensionContext.Namespace;

[...]

private static final Namespace NAMESPACE = Namespace.create(DefaultLocaleExtension.class);

This would mean we have to clean up other usages like in the RepeatFailedTestExtension where the namespace definition looks like the following:

private static final Namespace NAMESPACE = Namespace.create("org", "codefx", "RepeatFailedTestExtension");

In the vintage package there's a mix up

private static final Namespace NAMESPACE = Namespace.create("org", "junit-pioneer", "ExpectedException");

More meaningful tags

Some of our tags are just useless. Best example for this is the enhancement example. Everyhing is an enhancement, regardless if it's a new feature, a bugfix or a refactoring.
@nicolaiparlog mentioned in chat, that he initally used the tags of Junit. I've created a list which compares the tags (and tries to match them). In the time since the inital creation of our tags, JUnit has created a bunch of them. We should decide which of them we also want to use. An easy way would be to update my list and then change the tags ascynchronuous so we don't have to do this on stream

[Bukama]

#217 also effects clean up.

[Bukama]

Added a section in opening post about the github tags, based on chat last weekend.

[nipafx]

First of all, thank you for spotting these issues and taking the time to report and research them. I appreciate that.

Definition of annotations

I concur with everything except nesting the "plural" annotation inside the repeated one. This would mean that when users check an annotations source file, they show up inside the annotation they're interested in and that's a but weird. I was going to propose to put them as top-level classes in the same file, but while that improves that situation a bit, it doesn't entirely fix it.

As I see it, we have these options for the "plural" annotations:

1. their own file ~> straight-forward, yet cluttering and making it harder to track meta annotations
2. top-level, package-visible class in repeated annotation file ~> a bit weird, but less clutter and easy to see both annotations
3. inner class in repeated annotation file ~> a bit weird; see above
4. inner class in extension class file ~> a bit weird; hidden from Pioneer users (good), but making it harder to track meta annotations

I don't have an opinion, yet.

Location of annotations and other files

I agree that "everything in the same package" hurts and is already making it a bit uncomfortable to work with the code base. Over time, this will only get worse.

But splitting packages by, e.g., API, extensions, and utilities would mean that tons of things that are now package-visible need to be public. I don't want to expose the extension implementations and the utilities to our users if we can avoid it, not even if we add Javadoc or name one of the packages internal (ugh).

The only way to get smaller packages would be to split them by domain (as opposed to by code category). I would be open to that but don't quite see how we could come up with a categorization now that remains distinctive in the future.

I guess we could have system (for locale, time zone, env and sys vars), test (for repeat failed and report entries), and resource (for temp dir). Do we want to do that? (Utils still would have to be public.) Or just live with the clutter (it could be worse).

Naming convention

Good points all around, I'm on board with the following:

• if a class (indirectly) implements Extension, it should end with that word
• same for ArgumentsProvider

Note should, not must - there can be exception if well argued.

100% agree on the namespace, should be as in DefaultLocaleExtension.

More meaningful tags

Yes, the divergence is annoying and makes the otherwise not always ideal tag names pretty pointless. I agree we should update them.

Everybody interested should go through the list and mark in an extra column whether they think we should adopt a specific JUnit-5-tag or not. New ones have to be added as well (think upstream: Pioneer).

[Bukama]

Thanks for your response!

Definition of annotations

I concur with everything except nesting the "plural" annotation inside the repeated one. This would mean that when users check an annotations source file, they show up inside the annotation they're interested in and that's a but weird. I was going to propose to put them as top-level classes in the same file, but while that improves that situation a bit, it doesn't entirely fix it.

As I see it, we have these options for the "plural" annotations:

1. their own file ~> straight-forward, yet cluttering and making it harder to track meta annotations
2. top-level, package-visible class in repeated annotation file ~> a bit weird, but less clutter and easy to see both annotations
3. inner class in repeated annotation file ~> a bit weird; see above
4. inner class in extension class file ~> a bit weird; hidden from Pioneer users (good), but making it harder to track meta annotations

I don't have an opinion, yet.

I personally don't see the forth as good option as its clutters two things together in one class while having to two different tasks, just for hiding one.
The second and third option are quite close, but I prefered the third to hide the "plural" one completly (and not only for other packages). If someone checks the sourcefile, he/she will find quite the same construction anyway: Two annotations in one file and will stumble over the repeatable-array-thingy the one way or the other, when not used to it.

Aside from that, I think it's settled to put each "singluar" annotation into its own file, meaning at least @TempDirectoy has to be moved (maybe other).

Location of annotations and other files

I agree that "everything in the same package" hurts and is already making it a bit uncomfortable to work with the code base. Over time, this will only get worse.

But splitting packages by, e.g., API, extensions, and utilities would mean that tons of things that are now package-visible need to be public. I don't want to expose the extension implementations and the utilities to our users if we can avoid it, not even if we add Javadoc or name one of the packages internal (ugh).

The only way to get smaller packages would be to split them by domain (as opposed to by code category). I would be open to that but don't quite see how we could come up with a categorization now that remains distinctive in the future.

I guess we could have system (for locale, time zone, env and sys vars), test (for repeat failed and report entries), and resource (for temp dir). Do we want to do that? (Utils still would have to be public.) Or just live with the clutter (it could be worse).

You're unfair! Swinging the ultimate jOOQ-hammer.. cry

Joke aside: As mentioned on stream yesterday I honestly didn't notice that they are package visble and not public. That's make it better from the outside view and I agree on your concerns about making many things public. As I don't have a better idea (not even for domain packages) I would say we go for the current situation until we decide to make moduls or the clutter gets too heavy.

Naming convention

Good points all around, I'm on board with the following:

• if a class (indirectly) implements Extension, it should end with that word
• same for ArgumentsProvider

Note should, not must - there can be exception if well argued.

100% agree on the namespace, should be as in DefaultLocaleExtension.

Nice to see. Again I'll put TempDirecty onto the list, also to check and update the namespaces.

More meaningful tags

Yes, the divergence is annoying and makes the otherwise not always ideal tag names pretty pointless. I agree we should update them.

Everybody interested should go through the list and mark in an extra column whether they think we should adopt a specific JUnit-5-tag or not. New ones have to be added as well (think upstream: Pioneer).

Will check within the next days.

Settled todo so far:

• Move "singular" annotations to own files, e.g. @TempDirectoy
• Refactor TempDirectoy to meet naming convetions
• Update all extension namespace definitons to match private static final Namespace NAMESPACE = Namespace.create(<extensionname>.class);
• Document decisions somewhere (contributin.md maybe?) for further contributions.

[Bukama]

In #228 I started to work on the settled point. I did the first three but didn't start to document the naming conventions etc. as I wanted to wait for feedback where and how. My guess would be CONTRIBUTING.md. But where in there? Appendix or listing them in the "Fixing Bugs, Developing Features" chapter?

[Bukama]

From stream:

We want to discuss the naming of

• InvocationContext (RepeatFailedTestInvocationContext implements TestTemplateInvocationContext)
• ParameterResolver (RangedSourceProvider)
• ArgumentProvider

[Bukama]

More meaningful tags

Some of our tags are just useless. Best example for this is the enhancement example. Everyhing is an enhancement, regardless if it's a new feature, a bugfix or a refactoring.
@nicolaiparlog mentioned in chat, that he initally used the tags of Junit. I've created a list which compares the tags (and > > tries to match them). In the time since the inital creation of our tags, JUnit has created a bunch of them. We should decide which of them we also want to use. An easy way would be to update my list and then change the tags ascynchronuous so we don't have to do this on stream

Yes, the divergence is annoying and makes the otherwise not always ideal tag names pretty pointless. I agree we should update them.
Everybody interested should go through the list and mark in an extra column whether they think we should adopt a specific JUnit-5-tag or not. New ones have to be added as well (think upstream: Pioneer).

Will check within the next days.

Updated the list with my proposal.

[Bukama]

More more point we should have in focus: Java docs!

Pioneer JavaDocs point our that there are several classes (especially the "container plural repeatable annotation classes") without JavaDoc. But also the package-info is out of date (even in master) - Shall we really list all extensions there? We then have always to remember that when doing PR reviews!

[Bukama]

Open points:

1. Decision on where we place repeatable "plural" annotations
2. Decision on tags we want to use
3. Naming Convetion of InvocationContext, etc (see above)
4. Documentation (started in CONTRIBUTING.md)
5. JavaDocs

Points I call done

• Location of annotations and other files -> All in same package as of now.

I ask everyone (and especially @aepfli ;) ) to post their feedback up to new Tuesday (2020-05-05). Thank you.

[aepfli]

nice tagging me, now i need to read the wall of text you to left behind ;) - just kidding

definition of annotation

I do not have a strong opinion regarding that, although i believe a project is most easy to adopt and to follow, as long as you follow some defacto standards. As it makes it easier to adopt for others. hence that i will try to find some time and look into other projects, and what is the most common way. i understand that sometimes reducing the cluttering is a good thing, but if it is on cost of maintainability and usual navigation through a project, it might not be ideal.

Github tags

the list looks fine to me - to be honest, i am getting lost in all those tags all the time, and sometimes to many tags is also not a good idea. but so far the list looks good. i think i could label tickets properly. maybe we should define that there is at least a type, status and theme provided for each task. and when we see one, to enhance the issues with appropriate labels (which we obviously should do anyways)

naming convention

hmm what to say i am in favor of a convention, and sticking to defaults is always good. - we just have to be careful with some renamings regarding backwards compatibility.

not sure regarding documentation und javadoc

[aepfli]

regarding github tags, we might also want to start use good-first-issue as this is available and easily accesible via https://github.com/junit-pioneer/junit-pioneer/contribute

not sure how this is configured

[Bukama]

regarding github tags, we might also want to start use good-first-issue as this is available and easily accesible via https://github.com/junit-pioneer/junit-pioneer/contribute

not sure how this is configured

Where does this come from? At we use the up-for-grabs which is also linked in this "do you look for an open source project" site (which I don't find in Discord and Twitter history atm)

[aepfli]

This contribute page is a GitHub default to allow easier ways into the project. And they have the spec to use good-first-issue. I am not sure how the current issues on that page were found.

[Bukama]

This contribute page is a GitHub default to allow easier ways into the project. And they have the spec to use good-first-issue. I am not sure how the current issues on that page were found.

okay it's machine learning...maybe we should contact Github and add tell them to search for "up-for-grabs" too :D

https://github.blog/2020-01-22-how-we-built-good-first-issues/

[Bukama]

As no further comments were added about the labels I updated them according to my suggestion (link see above)

[Bukama]

If #260 gets merged I think this issue is done. Targeted tasks:

Open points:

1. Decision on where we place repeatable "plural" annotations

Definition of annotations

I concur with everything except nesting the "plural" annotation inside the repeated one. This would mean that when users check an annotations source file, they show up inside the annotation they're interested in and that's a but weird. I was going to propose to put them as top-level classes in the same file, but while that improves that situation a bit, it doesn't entirely fix it.
As I see it, we have these options for the "plural" annotations:

1. their own file ~> straight-forward, yet cluttering and making it harder to track meta annotations
2. top-level, package-visible class in repeated annotation file ~> a bit weird, but less clutter and easy to see both annotations
3. inner class in repeated annotation file ~> a bit weird; see above
4. inner class in extension class file ~> a bit weird; hidden from Pioneer users (good), but making it harder to track meta annotations

I don't have an opinion, yet.

I personally don't see the forth as good option as its clutters two things together in one class while having to two different tasks, just for hiding one.
The second and third option are quite close, but I prefered the third to hide the "plural" one completly (and not only for other packages). If someone checks the sourcefile, he/she will find quite the same construction anyway: Two annotations in one file and will stumble over the repeatable-array-thingy the one way or the other, when not used to it.

Aside from that, I think it's settled to put each "singluar" annotation into its own file, meaning at least @TempDirectoy has to be moved (maybe other).

We are on the same line that the "plural" ones should not be public. The resulting two options (top level class or inside non-repeatable) are close. After more thinking about it I still vote for putting them inside the non-repeatable files together with a short comment. Doing this helps to reduce cluttering and when someone looks into the files we already has / tries to get further knowledge.

2. Decision on tags we want to use

Were updated, see comment above.

3. Naming Convetion of InvocationContext, etc (see above)
4. Documentation (started in CONTRIBUTING.md)

The CONTRIBUTING.md was slightly updated.

5. JavaDocs

With moving the repeatable annotations inside the non-repeatable ones, the main problem is gone. So I only updated the package-info to list all current annotations, so the file at least provides some "content". We have to be aware to update it in the future.