Skip to content

List.removeLast() docs error occurs if empty #35894

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

Closed

Conversation

camsteffen
Copy link
Contributor

Seems like this should be explicitly stated in the docs. I did not indicate the specific type of error because of #26087.

@lrhn
Copy link
Member

lrhn commented Mar 1, 2019

Thanks for the suggestion. I'll probably phrase it differently, but it should be said somehow.

@mraleph
Copy link
Member

mraleph commented Mar 10, 2019

@lrhn any ideas about alternative wording? Should this PR be closed if suggested wording does not satisfy your requirements?

@mraleph mraleph requested a review from lrhn March 10, 2019 18:39
@kevmoo kevmoo added the area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. label Mar 11, 2019
@ulidtko
Copy link

ulidtko commented May 2, 2019

I'd also like to see it documented which error exactly will happen. But, since #26087 is closed-not-planned (which means WONTFIX I guess), the error will indeed be of varying type.

I'd merge this PR easily as-is; for an extra mile of effort, I'd improve it further:

@@ -537,6 +537,8 @@ abstract class List<E> implements EfficientLengthIterable<E> {
/**
* Pops and returns the last object in this list.
*
* An error occurs if the list is empty.
Copy link
Member

@lrhn lrhn Jul 1, 2019

Choose a reason for hiding this comment

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

Just say:

 * The list must not be empty.

This is how we generally document prerequisites in the platform libraries. (We didn't always, which is why some documentation is not consistent, but this is the current official recommended approach.)

If a "must" requirement is not satisfied, the implementation will likely throw an Error, but there are situations where the throwing is best-effort only (mainly concurrent modification). We do allow JavaScript compiled code to rely on the underlying JavaScript throwing, so we don't want to enforce a specific Dart error to throw.

Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

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

Than you for the PR. I agree that it's a good idea to document the requirement.
We have a standard way of writing such prerequisites in platform library docs, so I have suggested that wording.

Copy link
Member

@lrhn lrhn left a comment

Choose a reason for hiding this comment

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

Than you for the PR. I agree that it's a good idea to document the requirement.
We have a standard way of writing such prerequisites in platform library docs, so I have suggested that wording.

@camsteffen camsteffen force-pushed the removelast-docs-empty branch from 74e4fce to 12969e1 Compare July 1, 2019 18:16
@camsteffen
Copy link
Contributor Author

I changed the wording as suggested.

@mit-mit
Copy link
Member

mit-mit commented Jul 4, 2019

@dart-bot dart-bot closed this in 0b1ec2d Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants