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

Add array slice method #31172

Merged
merged 1 commit into from
Sep 23, 2019
Merged

Conversation

creikey
Copy link
Contributor

@creikey creikey commented Aug 7, 2019

Closes #4715 if pythonic syntactic sugar is decided to be not needed.

@creikey creikey requested review from karroffel and a team as code owners August 7, 2019 05:39
@Chaosus Chaosus added this to the 3.2 milestone Aug 7, 2019
@@ -2933,6 +2933,17 @@
["const godot_pool_byte_array *", "p_pba"]
]
},
{
"name": "godot_array_slice",
Copy link
Contributor

@karroffel karroffel Aug 7, 2019

Choose a reason for hiding this comment

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

This needs to be part of the Core 1.2 extension and not placed in here, as this will cause binary compatibility issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my latest push I've incremented the core api version.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not what I meant 😄 I meant that this entry for godot_array_slice needs to live inside core/next/next/api (The Core 1.2 extension) instead of the core/api where it is currently located.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh, ok 😆 sorry my bad. I've moved the slice json object into the core/next/next/api section in my latest force push and gotten rid of the change in the python script.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 7, 2019

If you are adding the documentation, the deep argument should be documented too. Also delta should be wrapped in [code] block.

@Calinou
Copy link
Member

Calinou commented Aug 7, 2019

Would step be more appropriate than delta as the name of the third parameter?

Also, using a delta of 0 should print an error message (if I'm reading the code right).

@creikey creikey force-pushed the add-array-slicing branch 2 times, most recently from bfecd8d to 0797426 Compare August 7, 2019 23:20
@creikey
Copy link
Contributor Author

creikey commented Aug 7, 2019

@Calinou I've changed the name of the argument to step as delta is used in _process, and added an error print if zero is given as a step.

@KoBeWi I've explained the deep argument in the docs and wrapped the delta now step in a code block

@creikey creikey force-pushed the add-array-slicing branch from 0797426 to 5153b7a Compare August 7, 2019 23:38
@akien-mga akien-mga requested a review from bojidar-bg August 8, 2019 11:50
@creikey creikey force-pushed the add-array-slicing branch from 5153b7a to 684b44c Compare August 9, 2019 00:13
"minor": 0
},
"next": {
"core": {
Copy link
Member

Choose a reason for hiding this comment

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

Why change the indentation?

Copy link
Member

Choose a reason for hiding this comment

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

The commit log should also be amended now that it's no longer bumping core version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akien-mga In my latest force push I've fixed getting rid of the indentation and ammended the commit log.

"type": "CORE",
"version": {
"major": 1,
"minor": 1
"minor": 0
Copy link
Member

Choose a reason for hiding this comment

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

You're still changing the indentation, causing a diff while no actual change is made to the core definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@akien-mga I see the issue now, it was not showing up in my editor very clearly. I need to increase tab size for json... it has been fixed in the latest force push.

@akien-mga akien-mga merged commit 2114898 into godotengine:master Sep 23, 2019
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array slicing for GDScript
6 participants