Skip to content

feat: add configuration handler#129

Merged
Doomsk merged 5 commits into
hhat-lang:dev/rust_impl/pre0.3beta/arch_scaffoldfrom
tnemoz:dev/rust_impl/pre0.3beta/arch_scaffold
Jun 11, 2026
Merged

feat: add configuration handler#129
Doomsk merged 5 commits into
hhat-lang:dev/rust_impl/pre0.3beta/arch_scaffoldfrom
tnemoz:dev/rust_impl/pre0.3beta/arch_scaffold

Conversation

@tnemoz

@tnemoz tnemoz commented Jun 7, 2026

Copy link
Copy Markdown

Fixes #63. A base module has been added to config. This module contains the Config struct, which is to be used to read and write TOML configuration files using its to_file and from_file methods respectively. Devices are ordered according to whether they are active or not, and the new and from_file methods do check that at least one active device is present.

Note that I'm not entirely sure that this is what's expected for this issue, so don't hesitate to tell me what should be added/removed/changed!

Concerning the LLM use, VS Code autocomplete gave me reverse and find which I did not know about (I would have went for a less idiomatic code without it). The rest comes from me, the toml crate documentation, and SO because I never had to read from and write to files in Rust 😄

Generative AI/LLM disclosure

Code and Logic Architecture/Design:

  • The code and logic architecture/design contain no generative AI/LLM
  • The code and logic architecture/design are partially performed by generative AI/LLM
    • X% performed by the author(s)
  • The code and logic architecture/design are fully performed by generative AI/LLM

Code content:

  • The code contains no generative AI/LLM work
  • The code is partially written by generative AI/L
    • 90% performed by the author(s)
  • The code is fully written by generative AI/LLM

Code review:

  • The code review contains no generative AI/LLM
  • The code review is partially done by generative AI/LLM
    • X% performed by the author(s)
  • The code review is fully done by generative AI/LLM

Code tests:

  • The code tests contain no generative AI/LLM
  • The code tests are partially written by generative AI/LLM
    • X% performed by the author(s)
  • The code tests are fully written by generative AI/LLM

@Doomsk

Doomsk commented Jun 7, 2026

Copy link
Copy Markdown
Member

Hi @tnemoz thank you for this PR. I will start the review shortly.

@Doomsk Doomsk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @tnemoz for the work on this issue!
After the changes are done, I will take a look at some examples locally and test them against the code.

Comment thread rust/hhat_lang/src/config/base.rs Outdated
Comment thread rust/hhat_lang/src/config/base.rs
@tnemoz

tnemoz commented Jun 8, 2026

Copy link
Copy Markdown
Author

Hey @Doomsk!

I've just pushed a fix that:

  • modifies the visibility of all relevant objects;
  • puts the code inside an inner module that is then reexported, so that the tests module can only use the public API;
  • adds getter for devices, which is private, along with convenience functions to add, remove, or replace a device;
  • adds the corresponding tests.

Tell me if there's something I've missed!

@Doomsk Doomsk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's taking good shape! A few more comments that I would like your opinion on before giving the green light

Comment thread rust/hhat_lang/src/config/base.rs Outdated
Comment thread rust/hhat_lang/src/config/base.rs Outdated
@tnemoz

tnemoz commented Jun 9, 2026

Copy link
Copy Markdown
Author

Hey @Doomsk!

I've just pushed a commit related to our previous discussion. Notably:

  • I've removed the checks for at least one active device
  • I've added a private priority attribute for Device, and Config sorts them when created.

I've hesitated to make priority an Option, and defaulting to is_active.into(), but I figured that since Rust doesn't really have optional arguments, users were gonna have to write None anyway of they wanted the default behavior, which isn't really more convenient than doing them themselves. That, and this avoids writing Config's behavior on None values for priority, which makes the object simpler.

Tell me what you think!

@Doomsk

Doomsk commented Jun 9, 2026

Copy link
Copy Markdown
Member

Thanks! @tnemoz
question: should the Device contain the priority or the 'Config`? Because if it's internal to need you would need iterate over each one to figure out their priority. I failed to see how priority plays out on the methods also.

@tnemoz

tnemoz commented Jun 9, 2026

Copy link
Copy Markdown
Author

Thanks! @tnemoz question: should the Device contain the priority or the 'Config`?

I did wonder about this! My first thought was that the priority is more related to the Config than the Device, since it doesn't really makes sense that, taken alone, a Device has a priority.

On the other hand, a Device doesn't exist outside of its Config parent, and it feels weird to have an attribute priorities that points to the devices attributes, kind of having a double-list. This is why I eventually decided that priority was better placed in Device, but if you disagree, I don't mind changing it.

Because if it's internal to need you would need iterate over each one to figure out their priority.

I'm not sure to see the problem here. I'm not sure to see in which instances a user could want to access the priority without the Device. Even in this case, they could access it via config.get_devices().map(|device| device.get_priority()).collect() if I'm not mistaken, which should be fairly efficient. Or is there a use case I don't have in mind?

I failed to see how priority plays out on the methods also.

Upon creation, be it from the new method, or from a file, the Config object orders its Devices according to their priorities, so that, when deserializing, devices appear by order of priority (higher first). This sorting is kept consistent when adding a Device or replacing a Device. Basically, the invariant is that the Vec of Devices is always sorted out by priority.

@Doomsk

Doomsk commented Jun 9, 2026

Copy link
Copy Markdown
Member

@tnemoz that's a tricky point. The use does not have access to it, but the compiler does. The point is: how to define priority? by order of insertion? some defined value by whoever add a new device? Then how to avoid priority collisions (or just append one after the other with same values)?
When I thought of priority list being inside Config, I envisioned Device having a device_id that could be used to order the priority list.
or should just the appending order for active devices matter?

@tnemoz

tnemoz commented Jun 10, 2026

Copy link
Copy Markdown
Author

Hey @Doomsk ! I've added a small docstring to the priority attribute to document how it works. Tell em if this is not what you had in mind!

@Doomsk Doomsk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Then it's good to go!

Comment thread rust/hhat_lang/src/config/base.rs

@Doomsk Doomsk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @tnemoz ! Please comment on the issue so I can assign it to you and you can receive the bounty.

@Doomsk
Doomsk merged commit 998624d into hhat-lang:dev/rust_impl/pre0.3beta/arch_scaffold Jun 11, 2026
@github-project-automation github-project-automation Bot moved this from Backlog to Done in H-hat lang kanban Jun 11, 2026
@tnemoz
tnemoz deleted the dev/rust_impl/pre0.3beta/arch_scaffold branch June 11, 2026 12:01
@tnemoz

tnemoz commented Jun 11, 2026

Copy link
Copy Markdown
Author

Thanks @tnemoz ! Please comment on the issue so I can assign it to you and you can receive the bounty.

Done! Thanks for the guidance throughout!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[IMPL] configuration file for quantum device information

2 participants