LUCENE-10400: revise binary dictionaries' constructor in kuromoji #643

mocobeta · 2022-02-03T16:06:51Z

I drafted an idea to revise the dictionary constructors.

Main changes:

always use Class.getResourceAsStream() to load class resources. (don't delegate it to the parent class)
deprecate the constructors that switch the resource location (classpath/file path).
- instead, break it down into two constructors - one for the classpath (non-arg constructor) and another (one-arg constructor) for the external file path.
- the two constructors are exactly the same except for the way to locate the resource, I introduced an internal constructor that takes Suppliers that returns InputStream.

Each dictionary class now has two constructors:

A private no-arg constructor. This loads class resources by directly invoking Class.getResourceAsStream() without delegation. This covers most use-cases.
A public one-arg constructor. This loads external resources from the user's file system. For advanced users.
- CharacterDefinition doesn't have this public constructor since this loads only class resources.

Also, each class (except for CharacterDefinition) has an internal auxiliary constructor that is invoked from the two entry-point constructors and populates instance fields from the given input streams (created from classpath or file path, or anything else).

mocobeta · 2022-02-04T02:59:33Z

@uschindler @msokolov
Would you take a look at the revised constructors' API design when you have some time? If this gains basic consensus, I will add tests for the newly introduced APIs.

uschindler · 2022-02-04T09:52:05Z

Hi, I am a bit busy, let me check later! Could take over weekend.

uschindler

Not my full review, just something I already noticed last night:

uschindler · 2022-02-04T09:54:21Z

lucene/analysis/kuromoji/src/java/org/apache/lucene/analysis/ja/dict/ConnectionCosts.java

+   * @param resourceLocation where to load resources from
+   * @throws IOException
+   */
+  public ConnectionCosts(String resourceLocation) throws IOException {


This should take Pathas Parameter, no String. According to Robert, Paths.get() should be put on forbidden list, as it makes testing with custom path impls impossible.

I changed the interfaces to take the valid Path objects.
7841817

uschindler · 2022-02-04T10:21:30Z

One thing that I wanted to add: We should add @deprecated(forRemoval=true), this makes builds very noisy if you use the deprecated API. By adding a since="9.1" tag we may also remove it earlier than 10.0.

Of course the deprecated APIs should not merged into "main", but maybe it is easier to keep them in in first commit and rmove in a secondary PR afterwards only in main.

mocobeta · 2022-02-04T10:27:34Z

One thing that I wanted to add: We should add @deprecated(forRemoval=true), this makes builds very noisy if you use the deprecated API. By adding a since="9.1" tag we may also remove it earlier than 10.0.

Of course the deprecated APIs should not merged into "main", but maybe it is easier to keep them in in first commit and rmove in a secondary PR afterwards only in main.

Thanks, I was thinking the same thing. Mark as Deprecated the two-args constructors for removal on branch_9x, and remove them on main.

msokolov · 2022-02-04T15:26:14Z

lucene/analysis/kuromoji/src/java/org/apache/lucene/analysis/ja/dict/BinaryDictionary.java

+  }
+
+  protected static Supplier<InputStream> openFileOrThrowRuntimeException(Path path)
+      throws RuntimeException {


Let's use IOExceptionUnchecked

UncheckedIOException :-)

Changed to throw UncheckedIOException. Also, I added a utility interface to wrap the try-catch clauses here and there.
cb1c549

msokolov · 2022-02-04T15:27:20Z

lucene/analysis/kuromoji/src/java/org/apache/lucene/analysis/ja/dict/BinaryDictionary.java

+        InputStream dictIS = dictResource.get()) {
+      DataInput in = new InputStreamDataInput(mapIS);
+      CodecUtil.checkHeader(in, TARGETMAP_HEADER, VERSION, VERSION);
+      targetMap = new int[in.readVInt()];


can we read the targetMap in a separate function for better readability/modularity?

msokolov · 2022-02-04T15:29:24Z

lucene/analysis/kuromoji/src/java/org/apache/lucene/analysis/ja/dict/BinaryDictionary.java

+    this.resourceScheme = null;
+    this.resourcePath = null;
+
+    int[] targetMapOffsets = null, targetMap = null;


is it necessary to assign nulls to these? Could we assign directly to members below?

Hmm now I see we basically just copy-paste the big ugly method that was here before. I wonder if it's possible to share the same implementation -- in other words can we make the old constructor call this one?

Unfortunately, we can't call/delegate to other this() from the old constructor. A kind of chicken-egg problem is there - getClass() in it - this is needed to delegate to the newly added constructor that takes input streams, but it's a constructor so getClass() can't be invoked before this().

Instead, we can retire the old constructor and call this() or new super() in the implementation classes without changing public APIs. Anyway, it's not great to have the if-else for resource switching and the current super() call to delegate class resources loading; I think it is okay to remove the old constructor in the abstract BinaryDictionary right now?
9c2c971

msokolov · 2022-02-04T15:31:20Z

lucene/analysis/kuromoji/src/java/org/apache/lucene/analysis/ja/dict/BinaryDictionary.java

+                + sourceId);
+      targetMapOffsets[sourceId] = targetMap.length;
+
+      in = new InputStreamDataInput(posIS);


same here - can we split out into a separate function? I guess Java's handling of final makes it annoying to assign to these members in a function called from the constructor. Perhaps we can return a String[][] ?

Generally all fields and variables should be final, so I agree. In addition, assigning null to variables first and then reassigning a real value is bad code pattern! They should be final and only once assigned. This is a relic from the old code, we should clean it up.
For the special case where you need to pass a value to super.ctor, you can use a static method. All other code should be moved to ctor only. This is especially important for multithreading, because the ctor is ensured to be atomic.

I'd do this.
8bfc984

and maybe add synchronized
ecc4ee9

The synchronized is not needed if the method is private and only called from ctor.

Problematic are methods tha are public or public to subclasses or those which have sideeffects.

The synchronized is not needed if the method is private and only called from ctor.

I will remove "synchronized" here again.

…lier

mocobeta · 2022-02-05T10:11:37Z

Encountered an issue with @Deprecated(forRemoval = true). We can't call them even from test cases since @SuppressWarnings("deprecation") doesn't work when it is marked as "for removal", and then test compilation fails with warnings.

> Task :lucene:analysis:kuromoji:compileTestJava FAILED
/mnt/hdd/repo/lucene/lucene/analysis/kuromoji/src/test/org/apache/lucene/analysis/ja/dict/TestTokenInfoDictionary.java:81: warning: [removal] TokenInfoDictionary(ResourceScheme,String) in TokenInfoDictionary has been deprecated and marked for removal
    return new TokenInfoDictionary(ResourceScheme.FILE, dir.resolve(dictionaryPath).toString());

Do we have to change tests to get rid of the deprecated method calls?

uschindler · 2022-02-05T10:54:00Z

Encountered an issue with @Deprecated(forRemoval = true). We can't call them even from test cases since @SuppressWarnings("deprecation") doesn't work when it is marked as "for removal", and then test compilation fails with warnings.
> Task :lucene:analysis:kuromoji:compileTestJava FAILED
/mnt/hdd/repo/lucene/lucene/analysis/kuromoji/src/test/org/apache/lucene/analysis/ja/dict/TestTokenInfoDictionary.java:81: warning: [removal] TokenInfoDictionary(ResourceScheme,String) in TokenInfoDictionary has been deprecated and marked for removal
    return new TokenInfoDictionary(ResourceScheme.FILE, dir.resolve(dictionaryPath).toString());
Do we have to change tests to get rid of the deprecated method calls?

Hi,

as we want to remove the methods anyway (at least in master), fixing the tests would be a good idea. If we need to maintain test in 9.x testing the old methods, a @SuppressWarnings("removal") would be applicable.

uschindler · 2022-02-05T11:02:18Z

In general I like the idea here, but I would do one change: You already defined a new functional interface, but the public methods taking Supplyier. I am against uselessly wrapping exceptions just because the stupid old Java concept of "checked" exceptions.

My proposal would be:

Define a @FunctionalInterface public interface ResourceSupplier<T> { T get() throws IOException; } in Lucene's core module next to IOUtils (or maybe inside IOUtils). I say this, because we may use the supplier pattern now more often and when Nori is also changed, we would need the interface again.
The constructors take ResourceSupplier<InputStream>

If you still want to go the wrapping, you may also have a look at https://twitter.com/thetaph1/status/1487397163718135812
This shows that you can trick out Java compiler and convert functional interfaces (by casting them using generics) to allow them to throw checked exceptions. I had a similar problem like this in some large codebase using SQLException and many Function, Supplier, Consumer. The trick was to just make the exceptions invisible to the stupid javac compiler :-) - it is perfectly legal code and won't break as it follows the laguage spec. It is just casting Exceptions away.

mocobeta · 2022-02-05T12:24:49Z

I added @SuppressWarnings("removal") to tests that invoke deprecated methods for now - to keep the tests as-is on branch_9x. I will remove the obsolete methods and change tests in a separate patch (after backporting this).

Also, the suggested ResourceSupplier<T> interface works very well, thanks.
As for the wrapping checked exception trick (type erasure plays the main role there, maybe?), it's cool but seems too much for me.

uschindler · 2022-02-05T12:44:09Z

lucene/core/src/java/org/apache/lucene/util/IOUtils.java

+   * @see java.util.function.Supplier
+   */
+  @FunctionalInterface
+  public interface ResourceSupplier<T> {


Maybe rename this to IOSupplier<T> to be in line with the other interfaces before. I wasn't aware that there were already interfaces like this.

uschindler · 2022-02-05T12:50:44Z

lucene/core/src/java/org/apache/lucene/util/IOUtils.java

@@ -528,7 +528,7 @@ public static void fsync(Path fileToSync, boolean isDir) throws IOException {
  }

  /**
-   * A resource supplier function that may throw on IOException
+   * A resource supplier function that may throw an IOException


I think we should add a note that the consumer has to close the resource (eg., use try-with-resources) and the supplier can count on this.

I added a note here. 4e96ed4

dweiss · 2022-02-05T16:49:49Z

The trick was to just make the exceptions invisible to the stupid javac compiler :-) - it is perfectly legal code and won't break as it follows the laguage spec. It is just casting Exceptions away.

This isn't exactly right - it is legal bytecode but it isn't exactly playing well with the language. The calling code may expect your interface to never throw a checked exception and thus only catch a selected subset of unchecked ones - a checked rethrown exception violates this semantics. To be a bit more concrete, imagine if the library code had this:

void foo(Supplier<T> t) {
  try {
    doSomething(t.get());
    processed = true;
  } catch (RuntimeException | Error e) {
    exceptionHappened = true;
  }
}

From the Java language point of view this either returns normally or sets the exception flag - it is not possible to write Java code that bypasses both processed and exceptionHappened. But the sneaky-rethrows hack does allow the supplier to go directly up the stack, up until an exception handler for a checked exception is found. This is where the problem is - it may leave the library code in inconsistent state.

I agree the lack of any exception signature on streams is terrible but I prefer to have wrappers that rethrow an unchecked exception, with the original one as the cause - this is legal and leaves no space for surprises like the one above.

mocobeta · 2022-02-06T10:54:06Z

Assuming this is close to being ready for merge, I'm going to prepare a changes entry for 9.1 (for deprecated APIs with removal.) Once this is backported I will open another pull request for main to remove the obsolete APIs with another changes entry (for removed APIs.) Then will need to repeat the same for Nori.
I will keep this open for one or two days for further review.

uschindler · 2022-02-06T10:54:51Z

This isn't exactly right - it is legal bytecode but it isn't exactly playing well with the language.

Hi @dweiss: Let's not discuss this here, the whole thing does not apply to this PR. My recommendation was to add another functional interface to IOUtils (we already created them there).

I would never design my APIs in a way like Java streams API was done, so having a separate functional interface for IO actions is a good idea and was applied by Tomoko to this PR. So no need for further work.

The above tweet is more about working around problems with Java Streams API. We know for sure that Java Streams API internally handles exception in a correct way (using finally blocks), so the code example you posted does not apply there. To me, checked exceptions are a misconception of Java, but you can start flamewars about that. Another misconception was to let MethodHandle.invoke() throw Throwable. Thats another case where sneaky rethoring must be done to have nice and correct behaviour for the caller of the stuff you do internally with MethodHandles. But there it is no problem because you left Java Type System already! So inside methods that call MethodHandle.invoke(), it is recommended and perfectly fine to use rethrow, as long as you make sure the caller form public API gets correct exceptions (means you sneaky rethrow all excepions, but you declare the method to have the checked exceptions you know of). BTW, Java internally also rethrows like this in the lamda bootstraps (I rewrote it for Elasticsearch Painless language). So whenever you have lambdas or MethodReferences in your code, sneaky rethrowing is used in bootstraps extensive.

In general: Every cast is a misuse of the Java language, and so is this one. What we do here is just casting the exception type. So it is a misuse of type system (like every cast is), but it is not a misuse of the language. Its perfectly legal to do it. If you try out my above Sneaky class and look (with eclipse) at the types introduced for the generics type arguments, it is funny how intelligent the Java compiler is: If you have a functional interface throwing E extends Exception and the code does not throw any checked exception, Javac inserts RuntimeException into the type argument.

I already talked too much, my problem with Sneaky is that if you use such type casting, the Java Compiler won't allow you to catch checked Exceptions anymore, that's the worst thing that happens - but Sneaky works great if you make sure that the method itsself calling your erased Java streams is redeclaring throws IOException. The try...catch posted above is.... bad code anyways. Better use finally blocks to ensure something is executed for sure.

One last thing: The above try..catch also leads to surprises, if the method you call suddenly changes throws in a later version of the API and you did not recompile it. So try...catch blocks like this are a risky and errorprone/PMD warn you about such code.

uschindler

Looks fine now. I am glad that the horrible API is going away.

uschindler · 2022-02-06T11:16:06Z

lucene/analysis/kuromoji/src/test/org/apache/lucene/analysis/ja/TestJapaneseTokenizer.java

@@ -491,6 +491,7 @@ public void testUserDict3() throws Exception {
  }

  // Make sure loading custom dictionaries from classpath works:
+  @SuppressWarnings("removal")


This test will be obsolete in main, so we can remove it completely. It does not test anything useful. We may need to adda test that really loads a resource from a file.

Yes, I am going to handle this in the following PR.

dweiss · 2022-02-06T12:53:45Z

The try...catch posted above is.... bad code anyways. Better use finally blocks to ensure something is executed for sure.

I know it can be written differently, but that's not the point, Uwe - I just showed an example of what can be lurking in the code you can't/ don't control (including the streams implementation). Like I said, I got burnt on this once and I tried not to abuse the type system's loopholes ever since.

uschindler · 2022-02-06T13:48:10Z

The try...catch posted above is.... bad code anyways. Better use finally blocks to ensure something is executed for sure.

I know it can be written differently, but that's not the point, Uwe - I just showed an example of what can be lurking in the code you can't/ don't control (including the streams implementation). Like I said, I got burnt on this once and I tried not to abuse the type system's loopholes ever since.

That's OK. I just wanted to say: When working with MethopdHandles the general rule, also internally followed by the JDK code is to rethrow the original exception. If you have a method Handle you are the only one who knows from the inspection of the type system which Exceptions it really throws.

If you would me to enforce not use rethrow or type casting here, I'd like you to open many bug reports in Elasticsearch Painless and the JDK source code's implementations of StringConcatFactory and LambdaMetaFactory. Both of those just rethrow exceptions everywhere. When working with MethodHandles you have to accept that you have left the type system and have to implement the type system on your own -- sorry. There it does not help if you wrap with runtime exceptions, because then the caller no longer gets exceptions that were expected. The problem I see with RuntimeExceptions and wrapping them for nonsense is that the caller of your method doing this no longer sees original exceptions. So you have to find the balance. And my balance clearly says: Throw exactly what you get inside your complex method handles logic. It is only important to REDECLARE the Exceptions excpected in the public API you expose to outside (that's what JDK and Painess does). Internally it ignores type system (also for performance).

mocobeta · 2022-02-06T15:48:47Z

lucene/analysis/kuromoji/src/java/org/apache/lucene/analysis/ja/dict/TokenInfoDictionary.java

-  private TokenInfoDictionary() throws IOException {
-    this(ResourceScheme.CLASSPATH, null);
+  private static InputStream getClassResource(String suffix) throws IOException {
+    final String resourcePath = TokenInfoDictionary.class.getSimpleName() + suffix;


This "simple name + suffix" naming convention is ubiquitous in kuromoji (and nori). I am wondering if this is an appropriate method to locate resources. I think this question could pop up again when we closely look at BinaryDictionaryWriter#write().

I think this is at least fine for the "default loading" for our own resources. It should not be used for public resources (there we now only have constructors taking file streams, as it makes no sense for external files).

mocobeta · 2022-02-07T10:30:58Z

Thanks for reviewing, I'm going to merge this. I will open another pull request to remove the obsolete methods on main.

mocobeta · 2022-02-07T14:41:21Z

I opened #655.
I think the diff would be obvious - will merge it tomorrow.

dsmiley · 2022-02-09T05:43:37Z

lucene/core/src/java/org/apache/lucene/util/IOUtils.java

+   *
+   * @see java.util.function.Supplier
+   */
+  @FunctionalInterface


What aboutorg.apache.lucene.util.IOSupplier that already exists?

I didn't notice there is o.a.l.util.IOSupplier. It's confusing to have two interfaces with the same signature.

Also, there are other functional interfaces in o.a.l.util.IOUtils corresponding to Java stdlib's Consumer and Function.
I think it might be better to move all functional interfaces in o.a.l.util.IOUtils to top-level interfaces? @uschindler

I was also not aware that this interface already existed. Because all others were in IOUtils, so this was inconsistent before.

IMHO, I agree with @mocobeta - Move all of them to IOUtils. Separate classes bloat the Javadocs.

Let's open a separate issue and remove the top-level one.

I slightly prefer to have top-level functional interfaces o.a.l.util.[IOSupplier|IOConsumer|IOFunction] for visibility since we will use them more frequently in the future. And there are only two such existing functional interfaces in IOUtils, so moving them to top-level would not be difficult - what do you think about it?
But I'm not so familiar with the convention in there, and am fine with having all of them under o.a.l.util.IOUtils.

It seems there is a bit of disagreement in the API design. I think we'd need to solve this inconsistency in the core util's API to proceed with LUCENE-10400, I will open an issue.

Yes, both is fine. Actually the nested ones have a naming problem: "IOUtils.IOSupplier" has two times IO, so either "IOSupplier" or "IOUtils.Supplier".

Excuse me, I wrote the previous comment before seeing your comment.

I'm glad that we have reached an agreement here. Those are public APIs, though they may be mostly used in lambdas, so I think an issue for this clean-up is needed anyway? I'll open an issue and a PR for it.

I'm leaning towards class-per-file convention as well. Too many nested (public) classes makes my head spin.

I think this depends on the use case. If the functional interface should be used by many classes throughout Lucene's API, I agree with that.

The ones in IOUtils were originally only meant to be part of method signatures inside IOUtils. In this case, it is better to hide them inside the IOUtils class, because nobody would refer to them in import statements! You call a method in IOUtils and use the functional interface implicitely, the name does not matter as you never import or use the interface name directly). But as soon as many APIs like kuromoji use the functional interface, then it is better to place them as top-level classes. Full agreement.

I think that happened with IOUtils: At beginning the interfaces were meant as method parameters to some methods only, but now they graduated to top-level citicens.

Hi, could anybody review #673?

mocobeta · 2022-02-19T04:22:31Z

We need to apply the same changes here to Nori. I opened #693.

rewrite BinaryDictionary constructor in kuromoji

63ca8e7

mocobeta mentioned this pull request Feb 3, 2022

LUCENE-10393: Unify resource loader in kuromoji and nori #638

Closed

mocobeta added 2 commits February 4, 2022 01:23

fix bugs in locating resources

90c2b54

add a Deprecated annotation

d4c89c1

mocobeta changed the title ~~LUCENE-10400: revise to load dictionary resources in kuromoji~~ LUCENE-10400: revise constructors to load dictionary resources in kuromoji Feb 4, 2022

uschindler requested changes Feb 4, 2022

View reviewed changes

msokolov reviewed Feb 4, 2022

View reviewed changes

mocobeta added 9 commits February 5, 2022 11:52

use UncheckedIOException; add wrapper interface for input stream supp…

cb1c549

…lier

take valid Path object in constructors

7841817

add license header

873bb04

retire abstract BinaryDictionary's two-args constructor

9c2c971

remove unused variables

db1494e

split up fat BinaryDictionary constructor

8bfc984

add final

bfb1cda

make initialization methods synchronized

ecc4ee9

invoke new constructor from deprecated ctor

ea72ded

mocobeta added 4 commits February 5, 2022 20:26

remove synchronized from private static methods

bfd437c

add forRemoval and since elements to Deprecated

3cc46a3

cleanup old methods

12960dd

introduce IOUtils.ResourceSupplier and rewrite ctors to use it

d1986ea

fix typo

bd5894b

uschindler requested changes Feb 5, 2022

View reviewed changes

uschindler reviewed Feb 5, 2022

View reviewed changes

mocobeta added 2 commits February 5, 2022 22:03

rename ResourceSupplier to IOSupplier

d219d64

add note to javadocs

4e96ed4

Update github hunspell regression test to use JDK 17

72c9cab

mocobeta mentioned this pull request Feb 6, 2022

Update github hunspell regression test to use JDK 17 #651

Merged

mocobeta marked this pull request as ready for review February 6, 2022 04:37

uschindler approved these changes Feb 6, 2022

View reviewed changes

mocobeta added 4 commits February 6, 2022 23:56

add changes entry

d769795

Merge branch 'main' into jira/LUCENE-10400-cleanup-kuromoji-dictionary

785f3f3

add forRemoval=true to ResourceScheme enum

32aff3f

simplify private method signatures

5c6a77a

mocobeta commented Feb 6, 2022

View reviewed changes

mocobeta changed the title ~~LUCENE-10400: revise constructors to load dictionary resources in kuromoji~~ LUCENE-10400: revise binary dictionaries' constructor in kuromoji Feb 7, 2022

mocobeta merged commit e7546c2 into apache:main Feb 7, 2022

mocobeta deleted the jira/LUCENE-10400-cleanup-kuromoji-dictionary branch February 7, 2022 10:31

mocobeta added a commit that referenced this pull request Feb 7, 2022

LUCENE-10400: revise binary dictionaries' constructor in kuromoji (#643)

e4ad3c7

dsmiley reviewed Feb 9, 2022

View reviewed changes

This was referenced Feb 9, 2022

use existing o.a.l.util.IOSupplier #667

Closed

LUCENE-10420: Move functional interfaces in IOUtils to top-level interfaces #673

Merged

mocobeta mentioned this pull request Feb 19, 2022

LUCENE-10400: revise binary dictionaries' constructor in nori #693

Merged

mocobeta mentioned this pull request May 6, 2022

LUCENE-10558: Implement URL ctor to support classpath/module usage in Kuromoji and Nori dictionaries #868

Merged

asfimport mentioned this pull request Mar 22, 2022

Clean up the constructors' API signature of dictionary classes in kuromoji and nori [LUCENE-10400] #11436

Closed

LUCENE-10400: revise binary dictionaries' constructor in kuromoji #643

LUCENE-10400: revise binary dictionaries' constructor in kuromoji #643

Conversation

mocobeta commented Feb 3, 2022 • edited Loading

mocobeta commented Feb 4, 2022

uschindler commented Feb 4, 2022

uschindler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschindler commented Feb 4, 2022

mocobeta commented Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mocobeta Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mocobeta Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mocobeta Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mocobeta commented Feb 5, 2022

uschindler commented Feb 5, 2022

uschindler commented Feb 5, 2022

mocobeta commented Feb 5, 2022

uschindler Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dweiss commented Feb 5, 2022

mocobeta commented Feb 6, 2022

uschindler commented Feb 6, 2022 • edited Loading

uschindler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dweiss commented Feb 6, 2022

uschindler commented Feb 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mocobeta commented Feb 7, 2022

mocobeta commented Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mocobeta commented Feb 19, 2022

mocobeta commented Feb 3, 2022 •

edited

Loading

mocobeta commented Feb 4, 2022 •

edited

Loading

mocobeta Feb 5, 2022 •

edited

Loading

mocobeta Feb 5, 2022 •

edited

Loading

mocobeta Feb 5, 2022 •

edited

Loading

uschindler Feb 5, 2022 •

edited

Loading

uschindler commented Feb 6, 2022 •

edited

Loading

uschindler commented Feb 6, 2022 •

edited

Loading

mocobeta commented Feb 7, 2022 •

edited

Loading