Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check if multicodec table is in sync with the multiaddr table #142

Merged
merged 1 commit into from
Sep 30, 2022

Conversation

MarcoPolo
Copy link
Contributor

@MarcoPolo MarcoPolo commented Sep 28, 2022

Summary

Adds a simple check to make sure codecs defined in the multiaddr table are in sync with the multicodec table. See the github workflow run for this PR as an example.

Before Merge

  • Allow at least 24 hours for community input

@MarcoPolo
Copy link
Contributor Author

multiformats/multicodec#294 should fix the failing workflow.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test seems useful. I’m a bit hesitant since I’m not familiar with Clojure, and this code could become hard to maintain.

@MarcoPolo
Copy link
Contributor Author

This test seems useful. I’m a bit hesitant since I’m not familiar with Clojure, and this code could become hard to maintain.

Would you prefer bash instead ;) ?

Joking aside, I considered doing this in many other languages, but found the Clojure version more concise and simpler (and more fun!). The code is 60 lines, and there's no esoteric features here. However, if you want to write the Go version, feel free!

@marten-seemann
Copy link
Contributor

Would you prefer bash instead ;) ?

This could be a sharness test! 🤪

@MarcoPolo
Copy link
Contributor Author

CI now passes after multiformats/multicodec#294 was merged

@MarcoPolo MarcoPolo merged commit 9d0ba29 into master Sep 30, 2022
@MarcoPolo MarcoPolo deleted the marco/check-multicodec-in-sync branch September 30, 2022 20:23
@BigLep
Copy link

BigLep commented Oct 3, 2022

This test seems useful. I’m a bit hesitant since I’m not familiar with Clojure, and this code could become hard to maintain.

I think this is an important point. I'm worried about us introducing new languages without it being deliberate. JS or Python seem more likely for new folks to understand than Clojure.

@MarcoPolo
Copy link
Contributor Author

MarcoPolo commented Oct 3, 2022

I think this is an important point. I'm worried about us introducing new languages without it being deliberate. JS or Python seem more likely for new folks to understand than Clojure.

This is a fair point, and I should have done a better job explaining the design decision. Here's how I chose Clojure:

  • I didn't want bash.
    • It's deceptively tricky, and error prone.
  • I wanted something that could give me a lot out-of-the-box.
    • Clojure (and this Babashka interpreter) give you a lot out of the box. (persistent datastructures, data oriented semantics, pretty printing, a REPL)
  • Why not JS/Python/Go?
    • For JS, I would have preferred to use TypeScript. But that would involve an extra compile step or using a different runtime (e.g. Deno instead of Node).
    • I'm personally less familiar with Python, so this would have taken me a lot longer to get to the "Pythonic" way.
    • For Go, I wanted something that let me be more concise. This isn't a bad thing about Go in general, just not what I wanted here.
  • So if Clojure is the right tool for this 60 line script, why wouldn't we choose it?
    • Long term maintenance cost
      • I don't think this will be a problem since the format of these repos haven't changed in 5 years. The biggest change has been from having hex values to decimal values. Which would be a trivial change in this script.
    • Unfamiliar to others
      • This is the hardest for me to gauge since I'm already familiar with Clojure, but I have taught many folks Clojure in the past and onboarded many engineers to a Clojure codebase. It's always been pretty straightforward. But again this is hard to gauge.

So Clojure was chosen because of the combination of it being the right tool for the job and long term maintenance being negligible.

@BigLep
Copy link

BigLep commented Oct 3, 2022

Thanks for sharing @MarcoPolo - the reasoning makes sense.

Completely agreed about not using Bash.

I'm less-worried about this specific instance given this is not an area where we're we expect to be adding a lot more scripting.

My biggest worry is about the precedent as I want to make sure that introducing new languages and technologies is very deliberate and thought through from a long-term view.

I don't think we need to discuss more now, as long as we have the takeaway for an upfront conversation for the next time, especially if it is in an area we expect to build on or expand.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants