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

feat: fuzon-http implementation #20

Merged
merged 18 commits into from
Oct 2, 2024
Merged

feat: fuzon-http implementation #20

merged 18 commits into from
Oct 2, 2024

Conversation

cmdoret
Copy link
Member

@cmdoret cmdoret commented Oct 1, 2024

Adds a webserver to deploy fuzon as a microservice.
It has the following features:

  • load key: ontology pairs (aka collections) from a config file
  • cli to provide config file as parameter
  • /top endpoint to retrieve top N codes from a collection matching input text
  • /list endpoint to retrieve list of collections

@cmdoret cmdoret self-assigned this Oct 1, 2024
fuzon-http/README.md Outdated Show resolved Hide resolved
fuzon-http/src/main.rs Outdated Show resolved Hide resolved
Copy link
Member

@supermaxiste supermaxiste left a comment

Choose a reason for hiding this comment

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

praise: code looks clean. I tried to look for small improvements, but the code is simple and very self explanatory. Feels very Groovy somehow ;)
praise: installation works neatly and tool also runs smoothly 🥳

Major

issue: we need to be more transparent with the user about what is going on in the backend.

  1. A message stating which ports are exposed is a good start. A full link on how to access the open port is also very helpful, this way users know where things are happening and what is running.

  2. A more descriptive Usage section would be best because APIs are hard, especially when there's not a lot of documentation. Breaking down commands and giving examples is always a plus.

  3. We build fuzon with modos in mind as use case. How can a dev with a tool similar to modos set up and interact with fuzon via fuzon-http? I feel like having an example use case somewhere in the README would be nice. I'm happy to hear your thoughts

Minor

thought(non-blocking): we're currently hardcoding the port, should that be also part of the config somehow? As mentioned before, as long as we're transparent with the user, we can leave it as is for now.

suggestion(non-blocking): I know the code is not too long, but some unit tests for some of the functions would be great. It can be a separate issue as well.

@cmdoret
Copy link
Member Author

cmdoret commented Oct 2, 2024

Thanks for the excellent points @supermaxiste

I've addressed your comments except for:

A full link on how to access the open port is also very helpful

I could harcode messages into the code, but I think this would better be done through something like an openapi spec, which can be generated using rust packages (e.g. paperclip). But this is probably too big for this PR.

some unit tests for some of the functions would be great

Simply because most functions require the server to be online for testing and I'm not sure how to do it cleanly, there is prob a way to do this, but I'd rather keep it for another PR.

Copy link
Member

@supermaxiste supermaxiste left a comment

Choose a reason for hiding this comment

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

Very good changes, re-tested everything successfully and all works like a charm 👏

praise: the future-proof API structure is an excellent point I'll keep in mind
praise: README is great and the gif is great ⭐

For the unit tests & the API spec I'll open separate enhancement issues 👍

@cmdoret
Copy link
Member Author

cmdoret commented Oct 2, 2024

Thanks @supermaxiste indeed, let's tackle #21 and #22 separately 👍

@cmdoret cmdoret merged commit fdadf64 into main Oct 2, 2024
11 checks passed
@cmdoret cmdoret deleted the feat/fuzon-http branch October 10, 2024 21:14
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.

2 participants