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.

Sign up for GitHub

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

Jump to bottom

chore(stdlib)!: Use default arguments in more of stdlib #1772

Merged
merged 1 commit into from
Jan 29, 2024
Merged

chore(stdlib)!: Use default arguments in more of stdlib #1772

merged 1 commit into from
Jan 29, 2024

Conversation

alex-snezhko
Copy link
Member

Makes use of default arguments in various places of the stdlib where I thought they made sense.

I was also considering combining unwrap and expect in Option/Result but was unsure if I should.

Closes #697

phated
phated requested changes Mar 20, 2023
View reviewed changes
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

Quick skim, 2 things jumped out:

  1. graindoc needs to be fixed and you shouldn't be embedding defaults into the descriptions
  2. No shortened names for defaulted params, we want to be very clear with their names.

stdlib/map.gr Outdated Show resolved Hide resolved
stdlib/priorityqueue.gr Outdated Show resolved Hide resolved
spotandjake
spotandjake reviewed Mar 22, 2023
View reviewed changes
Copy link
Member

@spotandjake spotandjake left a comment

Choose a reason for hiding this comment

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

I didnt look over any of the test changes just the code changes but I am liking the look of this pr.

@phated phated added the blocked label Apr 13, 2023
@phated
Copy link
Member

phated commented Apr 16, 2023

blocked on #1776

@alex-snezhko
Copy link
Member Author

#1776 merged, no longer blocked

spotandjake
spotandjake approved these changes Jan 26, 2024
View reviewed changes
Copy link
Member

@spotandjake spotandjake left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good to me.

I don't think we should combine the unwrap and expect functions though.

ospencer
ospencer reviewed Jan 26, 2024
View reviewed changes
stdlib/array.gr Outdated Show resolved Hide resolved
phated
phated approved these changes Jan 29, 2024
View reviewed changes
Copy link
Member

@phated phated left a comment

Choose a reason for hiding this comment

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

This is an absolute delight! Thanks @alex-snezhko 🎉

stdlib/string.gr Show resolved Hide resolved
@phated
Copy link
Member

phated commented Jan 29, 2024

@alex-snezhko can you resolve the conflicts please?

@phated phated mentioned this pull request Jan 29, 2024
Open
@phated phated enabled auto-merge January 29, 2024 01:14
@phated phated added this pull request to the merge queue Jan 29, 2024
Merged via the queue into grain-lang:main with commit f5e934f Jan 29, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ospencer ospencer ospencer left review comments

@phated phated phated approved these changes

@spotandjake spotandjake spotandjake approved these changes

@peblair peblair Awaiting requested review from peblair peblair is a code owner

@marcusroberts marcusroberts Awaiting requested review from marcusroberts marcusroberts is a code owner

@jozanza jozanza Awaiting requested review from jozanza jozanza is a code owner

Assignees

@alex-snezhko alex-snezhko

Labels
None yet
Projects
No open projects
Grain v0.6.0
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BOM arguments to String.encode and String.decode should be optional
4 participants
@alex-snezhko @phated @ospencer @spotandjake