Skip to content

Refactor generated spec data to facilitate looking up by spec name#3817

Closed
westonruter wants to merge 5 commits intodevelopfrom
refactor/generated-spec-data
Closed

Refactor generated spec data to facilitate looking up by spec name#3817
westonruter wants to merge 5 commits intodevelopfrom
refactor/generated-spec-data

Conversation

@westonruter
Copy link
Copy Markdown
Member

@westonruter westonruter commented Nov 26, 2019

Summary

Fixes #4597.

Todo

  • There should be an efficient way to look up a spec by a spec name regardless of the tag it occurs in.
  • Instead of looping over the tag specs to find the one with the matching spec name, it would be better if the spec name was the array key.

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v1.5 milestone Nov 26, 2019
@googlebot googlebot added the cla: yes Signed the Google CLA label Nov 26, 2019
@westonruter westonruter changed the title Refactor generated spec data Refactor generated spec data to facilitate looking up by spec name Nov 26, 2019
@westonruter
Copy link
Copy Markdown
Member Author

  • There should be an efficient way to look up a spec by a spec name regardless of the tag it occurs in.

This is the best possible right now:

	/**
	 * Get tag by spec name.
	 *
	 * @since 1.5
	 *
	 * @param string $spec_name Spec name.
	 * @return array|null Allowed tag spec, or null if the tag does not exist.
	 */
	public static function find_tag_spec( $spec_name ) {
		foreach ( self::$allowed_tags as $tag_name => $allowed_tag ) {
			foreach ( $allowed_tag as $rule_spec ) {
				if (
					( ! isset( $rule_spec['tag_spec']['spec_name'] ) && $tag_name === $spec_name )
					||
					( isset( $rule_spec['tag_spec']['spec_name'] ) && $rule_spec['tag_spec']['spec_name'] === $spec_name )
				) {
					return array_merge( compact( 'tag_name' ), $rule_spec );
				}
			}
		}
		return null;
	}

We can eliminate looping over $allowed_tag by using the spec name as the array key. But then, ideally we wouldn't have to loop over self::$allowed_tags.

Only way I can think of to prevent this is having a separate index of spec_name mapping to tag/index.

@westonruter westonruter force-pushed the add/invalid-markup-reason branch from d03b803 to 5e56cce Compare November 26, 2019 18:58
@westonruter westonruter force-pushed the refactor/generated-spec-data branch from 2ac3dd4 to df10dfe Compare November 26, 2019 19:14
@westonruter
Copy link
Copy Markdown
Member Author

Linking to the relevant docs page on amp.dev for each violated tag spec depends on each tag spec having a spec_url defined. See ampproject/amphtml#26033.

@westonruter westonruter changed the base branch from add/invalid-markup-reason to develop December 16, 2019 06:07
@westonruter westonruter force-pushed the refactor/generated-spec-data branch from df10dfe to e56b2c3 Compare December 17, 2019 05:08
@westonruter
Copy link
Copy Markdown
Member Author

Closing in favor of the work that @schlessera is doing in #4566.

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

Labels

cla: yes Signed the Google CLA WS:Core Work stream for Plugin core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ability to look up spec by name

3 participants