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

refactor(const): Mark low level fns const #275

Merged
merged 1 commit into from
Jun 26, 2023

Conversation

a-kenji
Copy link
Contributor

@a-kenji a-kenji commented Jun 24, 2023

Mark functions of structs that suit themselves to be composed with multiple other widgets as const, namely:

  • Block
  • Layout
  • Rect

Continuation of: #115

This allows with some limitations to share blocks, layouts, through many widgets and with some luck also to remove lazy_static! from some of those use cases.

@codecov
Copy link

codecov bot commented Jun 24, 2023

Codecov Report

Merging #275 (65b1118) into main (bfcc550) will increase coverage by 0.22%.
The diff coverage is 86.51%.

❗ Current head 65b1118 differs from pull request most recent head 084bf41. Consider uploading reports for the commit 084bf41 to get more accurate results

@@            Coverage Diff             @@
##             main     #275      +/-   ##
==========================================
+ Coverage   82.50%   82.73%   +0.22%     
==========================================
  Files          36       36              
  Lines        7301     7360      +59     
==========================================
+ Hits         6024     6089      +65     
+ Misses       1277     1271       -6     
Impacted Files Coverage Δ
src/widgets/block.rs 84.15% <77.77%> (+0.85%) ⬆️
src/layout.rs 90.07% <94.87%> (+2.34%) ⬆️
src/style.rs 85.65% <100.00%> (+0.31%) ⬆️

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Can you please add unit tests? These help document the intent of this change and prevent the accidental removal of the const keyword later. These can be simple one liners that check that we can use these in a constant context. Something like:

fn layout_can_be_const() {
  const layout: Layout = Layout::default()
    .margin(1)
    .horizontal_margin(1)
    ...
// or 
  const layout: Layout = Layout::default().margin(1);
  const layout: Layout = Layout::default().horizontal_margin(1);
  ...
}

For a commit message that targets the changelog, this is not a refactor as it really enables some extra functionality (the ability to use the layout and block in a constant context. So perhaps:

feat(misc): allow layout and block to be used in constant context

Block can now be used like:
`const BORDER_BLOCK: Block = Block::default().borders(...`
Layout can now be used like:
`const MAIN_LAYOUT: Layout = Layout::default().margin(...`

@a-kenji
Copy link
Contributor Author

a-kenji commented Jun 25, 2023

@joshka, thank you for the review!

For a commit message that targets the changelog, this is not a refactor as it really enables some extra functionality (the ability to use the layout and block in a constant context.

Agreed! I went for consistency here initially, but I changed the commit message now.

I added unit tests in the vein you were suggesting.
I also added the same unit tests for the const Style changes, should I keep them in this commit or split them into it's own commit?

@joshka joshka enabled auto-merge June 25, 2023 23:57
This allows the following types to be used in a constant context:
- `Layout`
- `Rect`
- `Style`
- `BorderType`
- `Padding`
- `Block`

Also adds several missing `new()` functions to the above types.

Blocks can now be used in the following way:
```
const DEFAULT_BLOCK: Block = Block::new()
    .title_style(Style::new())
    .title_alignment(Alignment::Left)
    .title_position(Position::Top)
    .borders(Borders::ALL)
    .border_style(Style::new())
    .style(Style::reset())
    .padding(Padding::uniform(1));

```

Layouts can now be used in the following way:
``
const DEFAULT_LAYOUT: Layout = Layout::new()
    .direction(Direction::Horizontal)
    .margin(1)
    .expand_to_fill(false);
```

Rects can now be used in the following way:
```
const RECT: Rect = Rect {
    x: 0,
    y: 0,
    width: 10,
    height: 10,
};
```
@joshka joshka added this pull request to the merge queue Jun 26, 2023
Merged via the queue into ratatui:main with commit 7a6c3d9 Jun 26, 2023
@joshka
Copy link
Member

joshka commented Jun 26, 2023

I rebased on main and rewrote the commit message a little.
We've enabled the setting to verify commits using GPG, which blocks merging directly if you don't have that setup.
See https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification for details.

@a-kenji a-kenji deleted the constification branch June 26, 2023 05:09
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