Skip to content

♻️ Refactor: Rename strip-prefix to xssi-prefix#25946

Merged
lannka merged 1 commit intoampproject:masterfrom
samouri:strip-prefix-rename
Dec 10, 2019
Merged

♻️ Refactor: Rename strip-prefix to xssi-prefix#25946
lannka merged 1 commit intoampproject:masterfrom
samouri:strip-prefix-rename

Conversation

@samouri
Copy link
Copy Markdown
Member

@samouri samouri commented Dec 9, 2019

summary
Rename the strip-prefix attribute to the more specific xssi-prefix as suggested by @lannka.

@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Dec 9, 2019

Hey @ampproject/wg-caching, these files were changed:

  • extensions/amp-list/0.1/test/validator-amp-list.html
  • extensions/amp-list/0.1/test/validator-amp-list.out
  • extensions/amp-list/validator-amp-list.protoascii

@honeybadgerdontcare
Copy link
Copy Markdown
Contributor

@lannka have these been verified to not be in use? otherwise we need an i2d.

@lannka
Copy link
Copy Markdown
Contributor

lannka commented Dec 9, 2019

@samouri correct me. seems like it's a new feature that is not launched / announced yet?

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Dec 9, 2019

Poll!
React with a 👍 if you like this name change and 👎 if you preferred strip-prefix

@lannka
Copy link
Copy Markdown
Contributor

lannka commented Dec 9, 2019

The reason I prefer xssi-prefix is that it explains itself.
strip-prefix only explains its behavior, which I felt is too generic. That is not how we normally write a config.

@lannka
Copy link
Copy Markdown
Contributor

lannka commented Dec 9, 2019

To back up my argument, I give a real world analogy

If a frying pan is configurable, this will be a more readable config to regular users:

nonSticky: true

instead of

coatWithTeflon: true

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Dec 9, 2019

@lannka: I tend to agree with you, as long as xssi-prefix would never have a different use

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Dec 9, 2019

Yeah, lets do it this way. @jridgewell / @lannka: if you have the chance can you submit this? I still don't have merge access.

@samouri samouri force-pushed the strip-prefix-rename branch from 269315b to 46865d8 Compare December 10, 2019 13:47
@dreamofabear
Copy link
Copy Markdown

I think either name is fine, but I actually read nonSticky as mapping to stripPrefix since they're less specific. :)

@lannka lannka merged commit b19fbb9 into ampproject:master Dec 10, 2019
@lannka
Copy link
Copy Markdown
Contributor

lannka commented Dec 10, 2019

@choumx my bad try :-(

@samouri samouri deleted the strip-prefix-rename branch December 10, 2019 18:19
@samouri
Copy link
Copy Markdown
Member Author

samouri commented Dec 10, 2019

It might be more like a frying pan that was specifically meant for eggs, but could technically cook anything. Would you want your frying pan to have the function cookFood or cookEggs?

amaltas added a commit that referenced this pull request Dec 11, 2019
* cl/283618549 Revision bump for #25847

* cl/283654708 mandatory_parent can use spec_name in addition to tag_name

* cl/283852039 Introduce a invalid doctype error for amp validation.

* cl/283882898 n/a

* cl/283993894 Revision bump for #25197

* cl/284115876 Revision bump for #25870

* cl/284258503 Revision bump for #25889

* cl/284856390 Revision bump for #25946
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
* cl/283618549 Revision bump for ampproject#25847

* cl/283654708 mandatory_parent can use spec_name in addition to tag_name

* cl/283852039 Introduce a invalid doctype error for amp validation.

* cl/283882898 n/a

* cl/283993894 Revision bump for ampproject#25197

* cl/284115876 Revision bump for ampproject#25870

* cl/284258503 Revision bump for ampproject#25889

* cl/284856390 Revision bump for ampproject#25946
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants