Remove unused code

Removes unsused import

Some comments on this patch. Overall, it’s on the right path. I have nit picks below, let me know if you want to make changes yourself, or would prefer me to do them while merging.

• I think the real reason of this patch seems to be to add the event filter variant AnyPushRef. Most of the changes end up changing the way we deduce the type of change, but that only happens because of the new filter. Mentioning the new filter variant in the commit message of the first patch would be a good idea.

• The first commit has a couple of typos in the first line of the commit message: “CHange” and “percieves”. Not a big deal, but if you make other changes to the patch, it’d be nice to fix.

• Does the is_push_ref function need to be exported from the module? If not, remove the pub. If it is, please add a documentation comment (with ///) explaining how the function should be used.

  I know other exported functions don’t have that either, yet, but I want to make sure everything exported is documented. This will mean https://docs.rs/radicle-ci-broker/latest/radicle_ci_broker/ will be more useful for people using the CI broker as a library, e.g., when building new adapters.

  Even if not exported, the function should have a normal comment explaining what it does, with an example. This will help future maintainers of the code understand it correctly faster.

• Speaking of… Do I understand is_push_ref correctly: it checks if the argument (name) is a ref for a branch for some node name space: that is, the ref it matches refs/namespaces/<NID>/refs/heads/<BRANCH>, for some NID and branch name? If so, I think the code should check the string matches that pattern. This will be a little more work to parse, especially without using regular expressions. I’d like at least the following test cases to have confidence in the function:

  • argument doesn’t start with refs/namespaces/ (note trailing slash)
  • argument starts with refs/namespacesfoo/bar/refs/heads/main
  • the NID contains a slash
  • the NID is not followed by /refs/heads/ (note trailing slash)
  • that is not followed be text

  However, I think this should be a parser function:

    fn parse_ref(s: &str) -> Option<ParsedRef> { ... }
  

  where ParsedRef is an enum:

    enum ParsedRef {
        // A ref to a patch, the patch id is contained in the variant.
        Patch(Oid),
        // A ref to a branch, the branch name is contained in the variant.
        Push(String),
    }
  

  By making the function a parser we avoid the common problem of validating and then later parsing: if the validation code and the parsing code don’t agree, results tend to be unnecessarily hard to debug. This also means we only need to parse a ref in one place.

  The pre-existing is_patch_ref should be subsumed to the parser, of course.

  I should have thought about this much earlier, when the ref parsing was first added. That’s my fault, sorry. If you don’t want to rework the patch to do this, please tell me, and I can change it after the patch is merged.

• You add a status.json that a later commit in the same patch deletes. It’d be tidier to rebase the commits to not add the file. (git rebase -i makes this reasonably easy. Ask on Zulip if rebasing is new to you.)

• Likewise the removal of is_push_ref could be folded into the first commit in the patch.

Thanks for the feedback. It seems that there are some things to do. I will update the patch and let you know when it’s ready.

No GiHub Actions Workflows found.

Provide a parser function for parsing reference.

Currently supported handlers for puch and patch events.

No GiHub Actions Workflows found.

I have pushed the proposed changes. Please let me know if there is something more that I can do.

Do you think that we should also use regex at the rest of the functions inside the event.rs file (is_patch_update, is_patch_ref, push_branch)? Would happily add that too in this patch.

For the parser function, I implemented the changes is the second commit. Introduced a new type of error MessageError::NoEventHandler that will allow the broker to continue execution in case of there is no handler for the specific event type. Currently added support for such and push on patch events.

I am not sure if this is the approach we should follow, so please let me know if this is fine or if we have to refactor it.

No GiHub Actions Workflows found.

Some additional comments.

• A function is_foo(x) should return a true/false value to indicate whether x is a foo. I’d like the is_patch_ref and is_push_ref functions to be removed, and merged into the parse_r3f function. Those functions are only ever called from within event.rs, so it should be easy enough to change all call sites: first call parse_ref, and then check the result.
• In ci-broker.rs, matching the result of RequestBuilder::biuld_trigger could match on Err(MessageError::NoEventHandler) as one arm, instead of using if in the Err(e) arm. This would seem to me to be clearer and more idiomatic Rust.

With those changes I think this will be OK to merge.