Skip to content

Add --max_num_posts parameter to export command#15

Merged
schlessera merged 6 commits intowp-cli:masterfrom
drzraf:export-limit
Sep 24, 2017
Merged

Add --max_num_posts parameter to export command#15
schlessera merged 6 commits intowp-cli:masterfrom
drzraf:export-limit

Conversation

@drzraf
Copy link
Copy Markdown

@drzraf drzraf commented Aug 29, 2017

wp export --limit=<num>

@drzraf
Copy link
Copy Markdown
Author

drzraf commented Aug 29, 2017

about the "Warning: limit should be a positive integer" is simply that the current code validate() default values thus making impossible to have a default value outside the scope allowed to the user (what is desirable in case of integer where -1 means option disabled)
suggestion: validate first and merge default then.

Copy link
Copy Markdown
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

👍 Few minor things to clean up.

'end_date' => NULL,
'post_type' => NULL,
'post_type__not_in' => NULL,
'limit' => -1,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we use NULL here instead of -1? NULL is a more consistent pattern with the rest of the file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Based on the failed tests, it appears the warning also needs to accommodate for the default value.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Current code validation mechanism does not let check_*() functions distinguish between default value and value from user input.

'start_date' => null,
'end_date' => null,
'start_id' => null,
'limit' => -1,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ditto comment above about NULL.

}

private function limit() {
if ($this->filters['limit'] > 0) return "LIMIT {$this->filters['limit']}";
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This conditional should follow WordPress coding standards.

@drzraf
Copy link
Copy Markdown
Author

drzraf commented Aug 31, 2017

new commits (don't know if you received notification about this)

@schlessera
Copy link
Copy Markdown
Member

@drzraf Looks good so far, apart from the breaking test.

Travis shows that the EXPORT_FILE has not been defined: https://travis-ci.org/wp-cli/export-command/jobs/270271776#L521

This is because EXPORT_FILE is not a constant, but a value generated from an export and stored for reference later in the same test, like here: https://github.com/drzraf/export-command/blob/0043abfc98aa93b0186b334f792fcd7c6badc045/features/export.feature#L56-L57

@drzraf
Copy link
Copy Markdown
Author

drzraf commented Sep 5, 2017

added 6bb2850

Raphaël Droz and others added 2 commits September 5, 2017 16:08
This brings it more in line with `max_file_size`.
@schlessera
Copy link
Copy Markdown
Member

Renamed the argument from --limit to --max_num_posts, as discussed in Slack (https://wordpress.slack.com/archives/C02RP4T41/p1505840530000437).

@schlessera schlessera added the command:export Related to 'export' command label Sep 24, 2017
@schlessera schlessera added this to the 1.0.3 milestone Sep 24, 2017
@schlessera schlessera changed the title wp export --limit Add --max_num_posts parameter to export command Sep 24, 2017
@schlessera schlessera merged commit b57e5f9 into wp-cli:master Sep 24, 2017
@schlessera
Copy link
Copy Markdown
Member

Thanks for the pull request, @drzraf !

danielbachhuber pushed a commit that referenced this pull request Nov 18, 2022
Add --max_num_posts parameter to export command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

command:export Related to 'export' command

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants