Skip to content

Spring clean for the inputs module#471

Merged
tsmbland merged 3 commits intomainfrom
input_private
Apr 4, 2025
Merged

Spring clean for the inputs module#471
tsmbland merged 3 commits intomainfrom
input_private

Conversation

@tsmbland
Copy link
Copy Markdown
Collaborator

@tsmbland tsmbland commented Apr 4, 2025

Just a little tidy up of the input module. Mostly making things private that don't need to be public, and tidying some import statements. Feel free to tell me if any of this is silly

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.10%. Comparing base (a9fe89a) to head (bed782d).
Report is 4 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #471   +/-   ##
=======================================
  Coverage   95.10%   95.10%           
=======================================
  Files          35       35           
  Lines        4947     4947           
  Branches     4947     4947           
=======================================
  Hits         4705     4705           
  Misses        120      120           
  Partials      122      122           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tsmbland tsmbland marked this pull request as ready for review April 4, 2025 10:44
@tsmbland tsmbland requested a review from alexdewar April 4, 2025 10:44
Copy link
Copy Markdown
Member

@alexdewar alexdewar left a comment

Choose a reason for hiding this comment

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

Lots of good cleanups here. There's one thing to do with a macro that I think needs fixing before merging but otherwise all good.

Some things were marked as pub either by mistake or because they originally needed to be pub but then didn't after refactoring. The reason I marked all the submodules of input as pub though was because there's no way to get warnings about missing doc comments in private modules 😞. That said, all those warnings have been fixed now so there's not really any reason to leave them public (I guess we'll just want to be vigilant about documenting at least the important functions etc. for new code), so maybe let's just run with this.

Actually, even the top-level modules could be made private although there are still a couple with missing doc comments that we probably want to fix up first:

❯ git grep -n 'allow(missing_docs)'
src/commodity.rs:1:  #![allow(missing_docs)]
src/model.rs:2:  #![allow(missing_docs)]

Comment thread src/commands.rs
use crate::log;
use crate::output::create_output_directory;
use crate::settings::Settings;
use crate::{input::load_model, log};
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.

VS Code seems determined to format use statements like this sometimes. No idea why

Comment thread src/input/region.rs Outdated
macro_rules! define_region_id_getter {
($t:ty) => {
impl crate::input::region::HasRegionID for $t {
impl super::super::region::HasRegionID for $t {
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.

I think this should be left as is, otherwise it'll be looking for the region module relative to wherever the macro is imported rather than this file.

@tsmbland
Copy link
Copy Markdown
Collaborator Author

tsmbland commented Apr 4, 2025

Thanks! I didn't realise about the missing docs comments. That's a bit silly. Anyway, looks like we've been pretty good at this, and we have #219 and #216 to for the two top-level modules you identified above.

@tsmbland tsmbland merged commit 635a2f7 into main Apr 4, 2025
7 checks passed
@tsmbland tsmbland deleted the input_private branch April 4, 2025 14:04
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