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

Remove unused terms, taxonomies and categories code #8250

Merged
merged 12 commits into from
Aug 8, 2018
Merged

Remove unused terms, taxonomies and categories code #8250

merged 12 commits into from
Aug 8, 2018

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Jul 27, 2018 •
edited
Loading

This PR removes code that is no longer used.

Open Questions

  • How should we go with removing this code? Does this need to be signaled anywhere?

How has this been tested?

  • Automatted tests: unit and e2e.
  • Manually: tested that working with tags and categories still work (both within the post and in their wp-admin pages).
  • Search for uses of these methods within the codebase.

@oandregal oandregal self-assigned this Jul 27, 2018
@oandregal oandregal changed the title Remove terms and categories code Remove unused terms, taxonomies and categories code Jul 27, 2018
youknowriad
youknowriad reviewed Jul 27, 2018
View reviewed changes
packages/core-data/src/selectors.js Outdated
*
* @return {Array} Categories list.
*/
export function getTerms( state, taxonomy ) {
Copy link
Contributor

@youknowriad youknowriad Jul 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I guess we should probably leave the code for two releases and add a "deprecated" call to these unused selectors.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR updated to use deprecated.

@gziolo gziolo added the [Type] Breaking Change For PRs that introduce a change that will break existing functionality label Jul 28, 2018
@gziolo gziolo added this to the 3.4 milestone Jul 28, 2018
@pento pento modified the milestones: 3.4, 3.5 Jul 30, 2018
youknowriad
youknowriad reviewed Jul 31, 2018
View reviewed changes
packages/core-data/src/reducer.js Outdated
version: '13.6.0',
alternative: 'entities reducer',
plugin: 'Gutenberg',
} );
Copy link
Contributor

Choose a reason for hiding this comment

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

The reducers are not public API, so I'd prefer avoiding these two warnings.

Copy link
Member Author

Choose a reason for hiding this comment

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

aaf9e2e

What should we do with the resolvers?

packages/core-data/src/actions.js Outdated
@@ -13,6 +14,11 @@ import { castArray } from 'lodash';
* @return {Object} Action object.
*/
export function receiveTerms( taxonomy, terms ) {
deprecated( 'receiveTerms action', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the public API in the messages instead?

deprecated( 'wp.data.dispatch("core").receiveTerms', {
	version: '13.6.0',
	alternative: 'wp.data.dispatch("core").receiveEntityRecords',
	plugin: 'Gutenberg',
} );

Copy link
Member Author

Choose a reason for hiding this comment

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

f6e862c

packages/core-data/src/resolvers.js
@@ -24,6 +25,11 @@ import { getKindEntities } from './entities';
* progress.
*/
export async function* getCategories() {
deprecated( 'getCategories resolver', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these deprecated selectors are used in the core-blocks/README.md, should we update the docs?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we should. Good catch. Is this something you could help with? It's still fuzzy to me how one should work with the entities abstraction. Let me know if you won't have the availability and I'll do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done at ea38313

@youknowriad youknowriad mentioned this pull request Aug 1, 2018
Merged
gziolo
gziolo requested changes Aug 6, 2018
View reviewed changes
Copy link
Member

@gziolo gziolo 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.

We also need to add all those deprecations to https://github.com/WordPress/gutenberg/blob/master/docs/reference/deprecated.md. Is it possible to consolidate them into one entry there?

packages/core-data/src/actions.js Outdated
@@ -13,6 +14,11 @@ import { castArray } from 'lodash';
* @return {Object} Action object.
*/
export function receiveTerms( taxonomy, terms ) {
deprecated( 'wp.data.dispatch("core").receiveTerms', {
version: '13.6.0',
Copy link
Member

Choose a reason for hiding this comment

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

Typo, it should be 3.6.0. Actually, we should bump to 3.7.0 as we had new release in the meantime.

packages/core-data/src/selectors.js Outdated
@@ -36,6 +37,11 @@ function isResolving( selectorName, ...args ) {
* @return {Array} Categories list.
*/
export function getTerms( state, taxonomy ) {
deprecated( 'wp.data.select("core").getTerms', {
version: '13.6.0',
Copy link
Member

Choose a reason for hiding this comment

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

13.6.0 should be replaced with 3.7.0 everywhere.

@oandregal oandregal force-pushed the remove/unused-code branch 2 times, most recently from bc6c3c9 to 6925f39 Compare August 6, 2018 09:41
@oandregal
Copy link
Member Author

@gziolo @youknowriad updated docs, version, and rebased from master.

@gziolo
Copy link
Member

gziolo commented Aug 7, 2018

There are 3 unit tests failing. It looks like you don't assert for the warnings introduced with deprecations. In general, we have configured out tests to ensure that printing to the console is under control. Tests will fail unless you explicitly assert for it. when you see:

expect(jest.fn()).not.toHaveWarned(expected)
Expected mock function not to be called but it was called with: ...

it means you need to call:

expect( console ).toHaveWarned();
// or more detailed
expect( console ).toHaveWarnedWith( 'The message printed to the console' );

@oandregal
Copy link
Member Author

@gziolo Thanks for letting me know that! 020ef51 fixes it. I've also rebased and fixed some conflicts in the deprecated.md file.

@gziolo
Copy link
Member

gziolo commented Aug 8, 2018

Thanks for removing those methods 👍

@gziolo gziolo merged commit ae2d929 into master Aug 8, 2018
@gziolo gziolo deleted the remove/unused-code branch August 8, 2018 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youknowriad youknowriad youknowriad left review comments

@gziolo gziolo gziolo approved these changes

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

Assignees

@oandregal oandregal

Labels
[Type] Breaking Change For PRs that introduce a change that will break existing functionality
Projects
None yet
Milestone
3.5
Development

Successfully merging this pull request may close these issues.
4 participants
@oandregal @gziolo @youknowriad @pento