srcrr-main-1 #1

What tool creates this file? Chances are we don't want to commit it; it might be worth adding to your global gitignore configuration.

I think that's added by goenv. I'll add it to gitignore.

I haven't heard of goenv before. It's not entirely clear to me what the benefit is since you can just install multiple versions of Go side-by-side and there's an officially supported method for doing so, but if it works for you it works I guess :)

I wouldn't add it to this projects gitignore, we don't want to pollute it with tons of random tools, but it might be a good idea to add it to your machines global gitignore or your account level gitignore.

This dependency is somewhat problematic as it's GPLed, so we'll have to be very careful not to vendor it or redistribute it. I think it's okay to depend on it now for the purpose of development, but long term we'll have to evaluate it for security, maintenance, etc. (this is more a note to self than anything). It hasn't seen a commit in a while, and part of this project is having a fully open/maintained stack for OMEMO, so it may be that we have to replace this before we can call it complete. We can chat about it in the room more later though; it's fine for now, I just wanted to mention it as part of this quick initial first pass at a review.

As previously discussed this appears abandoned. We'll need to decide what to do about it before calling this project done. No rush or anything, I just want this noted in the review.

Ah, whoops, thought I removed it!

Thanks, I'll take care of it.

Not strictly a review comment, I just wanted to say that I wasn't aware of this project and one of the tools in it may be very useful for reducing boilerplate later; nice!

Eventually this library will be named omemo (but see general comments). This means this (and other structs and methods) will be used by writing, eg. omemo.OmemoSignedPk which is redundant. We can remove the Omemo prefix since, unlike C et al, we already have a package name built in to the name of the type.

Also, Go convention is to keep acronyms in the same case so eg. this should actually be called SignedPK (or OMEMOSignedPK if we were keeping the current name).

This newline means the docs won't be rendered.

Two quick comments on the docs:

• Convention is for us to use line beginning comments, eg. // (this is a serious nit, we can fix it later, I just want to bring it up for future reference)
• More importantly, comments should always start with the name of the thing being commented on. Eg. "ImportBundle marshals XML into a bundle" or something along those lines. This means that when you grep for the identifier in the command line style docs you find the beginning of the doc string first which makes things much nicer to look up.

I know this is a generated file, but if it provides a way to remove these comments or provide actual docs that would be ideal. If not, no worries, we can improve this later.

This file appears to be empty and not doing anything; it can be removed :)

Java style docs won't be rendered and are unnecessary; if this was copied from somewhere else we need to review it for license compliance and make sure we mention that.

Not copied from somewhere else. I'll review proper golang doc style.

This sort of thing shouldn't be exported most likely, but we can do a full API review later. I'd also note that this looks pretty inefficient to me and I assume this is a fast-path function. That being said, this is also something we can improve later, so it may be that we just add a TODO comment that we should do this with simple slice manipulation and let the compiler handle it all for us. No worries either way, I'm just being nitpicky and leaving myself notes for later more than anything here.

This package needs a more descriptive name, I have no idea what "t" is :) Just glancing at it it may suffer from "tiny package syndrome" and not require its own package too, but I haven't looked at functionality yet so I'm unsure.

It's typing or type. I was finding the different subpackages were running into circular imports so I created a separate type package to resolve this.

And since I'm using these types all over the place I'm lazy and didn't want to continue to type type, so figured t would be easier. But, yeah, it is ambiguous. I'll refactor it to type.

If this is the case chances are we need to do a refactor, not create a new package. This is normally an indicator that there's a structural problem that we need to resolve. Glancing at this package, I'm not sure most of these "generic" types are super useful, so we should probably just remove it, but let's talk about each individual type and why it's needed and see, then maybe we can find a better place for them to live that doesn't involve generic "utility" packages (util, misc, types, etc. generally speaking turn into dumping grounds later that are hard to navigate and work on).

This package should likely be in internal/testutils so that it can't be imported outside of this module

This package should be in internal/conster to start. Probably no need for the tooling directory, it will be clear that it's a command from the docs and from the package name (main).

We should remove this type; most of the functionality is implemented in the bytes package. I can't provide a good rationale, but having a special type just to add methods instead of using Go's built in functions and slice manipulation feels wrong to me and I suspect it's going to cause problems later.

This function is being re-defined under a different name and should be inlined.

nit: these should be KDFEncryptionKey and KDFAuthKey per Go's naming conventions.

nit: none of this is project specific, it should all be in your system or user level gitignore file. Let's not include it here; I mean, these types of files definitely shouldn't be committed, but we don't want to maintain a giant list of every possible filetype in every repo.

.out and .test make sense since they're more specific to this project (or to Go projects specifically).

Gotcha.

When I start a project I basically like to copy the gitignore language-specific files from https://github.com/github/gitignore/ (saves on brain power), but I'll remove them.

Yah, I don't really like their habit of polluting random repos with files that obviously shouldn't be there globally; not the end of the world though. Thanks!