Initial Structure and Buildout of JBI

[bsieber-mozilla]

No description provided.

[leplatrem]

How does this relate to #1 ?

[grahamalama]

Looks like a solid start! A few inline comments, and 3 general things:

• I'm noticing that we're using assertions in places where regular exceptions might be a better choice. I'll also point out at least one or two places where I noticed it, but I think it might be a bit of a smell to use assert in production code. Granted, there are places where asserts can be useful, so I'm open to pushback here, but in general it's something I (personally) would avoid using

• As we kind of discussed on our call, I think some of the subpackages we're creating might not be needed just yet. Looking at it a bit more, I think all of the functions in the core modules could live nicely in src.jbi.process. I think that module has one clear job -- process the tags and actions to prepare them to be used by the FastAPI app.

• In places where you use type hints for container objects, it might be nice to further provide the type(s) for the objects in the container. E.g. Tuple[str, str]

[bsieber-mozilla]

How does this relate to #1 ?

@leplatrem I met with graham yesterday discussing most of this PR; and will again briefly tomorrow for some general review of PR 4.

#1 is still the primary; modifications and comments here will result in changes to #1.

This split was to reduce the amount of files viewed per-review. You can ignore #3 and #4 since it's the split history of #1

A future approach would be to ask for the review sooner--or split based on files/directories instead of commit history.

[bsieber-mozilla]

Looks like a solid start! A few inline comments, and 3 general things:

* I'm noticing that we're using assertions in places where regular exceptions might be a better choice. I'll also point out at least one or two places where I noticed it, but I think it might be a bit of a smell to use `assert` in production code. Granted, there are places where asserts can be useful, so I'm open to pushback here, but in general it's something I (personally) would avoid using

* As we kind of discussed on our call, I think some of the subpackages we're creating might not be needed just yet. Looking at it a bit more, I think all of the functions in the `core` modules could live nicely in `src.jbi.process`. I think that module has one clear job -- process the tags and actions to prepare them to be used by the FastAPI app.

* In places where you use type hints for container objects, it might be nice to further provide the type(s) for the objects in the container. E.g. `Tuple[str, str]`

@grahamalama thanks for the review! I will respond to each comment individually (and absorb them into #1).

[bsieber-mozilla]

• I'm noticing that we're using assertions in places where regular exceptions might be a better choice. I'll also point out at least one or two places where I noticed it, but I think it might be a bit of a smell to use assert in production code. Granted, there are places where asserts can be useful, so I'm open to pushback here, but in general it's something I (personally) would avoid using

Modified to use ValueError, ActionError, ConfigError, and ProcessErrors.

• As we kind of discussed on our call, I think some of the subpackages we're creating might not be needed just yet. Looking at it a bit more, I think all of the functions in the core modules could live nicely in src.jbi.process. I think that module has one clear job -- process the tags and actions to prepare them to be used by the FastAPI app.

I'm hesitant to re-add this together; but I will do a bit more thinking on this.

• In places where you use type hints for container objects, it might be nice to further provide the type(s) for the objects in the container. E.g. Tuple[str, str]

Updated where this was noticed.

---

Closing this PR as most of the review has been absorbed into #1