Skip to content

Convert more examples and tests to record! macro#10840

Merged
sholderbach merged 78 commits intonushell:mainfrom
sholderbach:example-record-macro
Oct 28, 2023
Merged

Convert more examples and tests to record! macro#10840
sholderbach merged 78 commits intonushell:mainfrom
sholderbach:example-record-macro

Conversation

@sholderbach
Copy link
Copy Markdown
Member

@sholderbach sholderbach commented Oct 25, 2023

Description

Use record! macro instead of defining two separate vec! for cols and vals when appropriate.
This visually aligns the key with the value.
Further more you don't have to deal with the construction of Record { cols, vals } so we can hide the implementation details in the future.

State

Not covering all possible commands yet, also some tests/examples are better expressed by creating cols and vals separately.

User/Developer-Facing Changes

The examples and tests should read more natural. No relevant functional change

Bycatch

Where I noticed it I replaced usage of Value constructors with Span::test_data() or Span::unknown() to the Value::test_... constructors. This should make things more readable and also simplify changes to the Span system in the future.

Copy link
Copy Markdown
Contributor

@fdncred fdncred left a comment

Choose a reason for hiding this comment

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

Looks good standardizing to record!()

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 26, 2023

here's the next 1000 commits. lol

Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

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

wow that sounds tideous, you were extra brave on that one @sholderbach 🙏

i scrolled quickly the changes and they looked ok from far away, i think we can trust you on that one 😌

@amtoine
Copy link
Copy Markdown
Member

amtoine commented Oct 26, 2023

@sholderbach
do you happen to know which commands are still using Record { ... } explicitely?

@sholderbach sholderbach marked this pull request as ready for review October 28, 2023 12:23
@sholderbach
Copy link
Copy Markdown
Member Author

Done. No more manual Record construction in the tests as far as I can search for.

@sholderbach sholderbach merged commit 4b30171 into nushell:main Oct 28, 2023
@sholderbach sholderbach deleted the example-record-macro branch October 28, 2023 12:52
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Oct 28, 2023

wow, what a marathon! great job!

hardfau1t pushed a commit to hardfau1t/nushell that referenced this pull request Dec 14, 2023
# Description
Use `record!` macro instead of defining two separate `vec!` for `cols`
and `vals` when appropriate.
This visually aligns the key with the value.
Further more you don't have to deal with the construction of `Record {
cols, vals }` so we can hide the implementation details in the future.

## State

Not covering all possible commands yet, also some tests/examples are
better expressed by creating cols and vals separately.

# User/Developer-Facing Changes
The examples and tests should read more natural. No relevant functional
change

# Bycatch

Where I noticed it I replaced usage of `Value` constructors with
`Span::test_data()` or `Span::unknown()` to the `Value::test_...`
constructors. This should make things more readable and also simplify
changes to the `Span` system in the future.
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
# Description
Use `record!` macro instead of defining two separate `vec!` for `cols`
and `vals` when appropriate.
This visually aligns the key with the value.
Further more you don't have to deal with the construction of `Record {
cols, vals }` so we can hide the implementation details in the future.

## State

Not covering all possible commands yet, also some tests/examples are
better expressed by creating cols and vals separately.

# User/Developer-Facing Changes
The examples and tests should read more natural. No relevant functional
change

# Bycatch

Where I noticed it I replaced usage of `Value` constructors with
`Span::test_data()` or `Span::unknown()` to the `Value::test_...`
constructors. This should make things more readable and also simplify
changes to the `Span` system in the future.
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