feat: migrate format from `@guardian/types` #220

jamie-lynch · 2021-06-23T14:07:48Z

What does this change?

Migrates format from @guardian/types to @guardian/libs
Adds Design.Obituary (https://github.com/guardian/types/pull/151/files)

Why?

See guardian/types#136

jamie-lynch · 2021-06-23T14:09:07Z

cc @JamieB-gu

coveralls · 2021-06-23T14:09:46Z

Coverage increased (+0.06%) to 99.512% when pulling de02427 on migrate-format into cf32636 on main.

jamie-lynch · 2021-06-23T14:46:35Z

src/format.test.ts

+import { Design, Display, Pillar, Special } from './format';
+
+it('Design enum contains Article', () => {
+	expect(Design.Article).toBeDefined();
+});
+
+it('Display enum contains Standard', () => {
+	expect(Display.Standard).toBeDefined();
+});
+
+it('Pillar enum contains News', () => {
+	expect(Pillar.News).toBe(0);
+});
+
+it('Special enum contains SpecialReport', () => {
+	expect(Special.SpecialReport).toBe(5);
+});


This keeps coveralls happy but I'm not sure that it's much use beyond that. Interested to hear any thoughts on a better approach or whether it's just not worth it.

Ah, interesting that coveralls is really keen for you to run tests on what are essentially just type definitions - there's not really any logic to test there.

That said, testing the Theme enum might be useful, it's built as a compound of two other enums so I guess there's a chance for something to get broken there.

Otherwise, yeah I agree, this seems reasonable just to keep coveralls happy, but it's probably just running over ground that TSC covers anyway.

Yes, it seems Jest will always report enums as uncovered, unless maybe you use these enums somewhere?

They're actually good tests to have. It's not unfeasible that someone might modify the type and accidentally modify an existing Type due to a typo, or change the ordering and have that somehow break something

JamieB-gu

Nice! Thanks for doing this 🚀

One comment about obituaries - otherwise looks good!

src/format.ts

src/format.README.md

Co-authored-by: Jamie B <[email protected]>

sndrs · 2021-06-28T16:28:27Z

src/format.README.md

+
+```js
+import type { Theme, Format } from '@guardian/libs';
+import { Pillar, Special, Design, Display } from '@guardian/libs';


i think we need to think about these names here. i know they have big significance in editorial, but examples like this:

import { Special } from '@guardian/libs'; import { Design } from '@guardian/libs';

need to convey more to non-editorial devs i think. esp when it pops up in a list of possible imports in autocomplete (and in a generic org-wide library).

@JamieB-gu @oliverlloyd does something like this make sense?

import type { EditorialTheme, EditorialFormat } from '@guardian/libs'; import { EditorialPillar, EditorialSpecial, EditorialDesign, EditorialDisplay } from '@guardian/libs';

I see your point. Given that libs is dept-wide and not Editorial-specific the names do lose some context. Perhaps this is the best solution 🤔.

Although it's tricky because by making it more specific for non-consumers of these exports you're making it verbose/overly specific for the applications that do consume them. I don't have an idea for how to solve that though 😔.

Given that these may not live in this library for long do we want to do the large find-and-replace PRs on DCR, AR and IR to accommodate this change?

Apologies @sndrs none of what I've said is particularly conclusive, I've just added some more questions 🙈

cc @gtrufitt

Yeah, import { Special } from '@guardian/libs'; lacks a lot of context outside of our bubble 🤔

EditorialSpecial is explicit but also a bit clunky. You'd probably end up import { EditorialSpecial as Special } from '@guardian/libs'; which I guess is fine but also a bit meh.

I'm guessing not but Is there an option to use a folder, like

import { Special } from '@guardian/libs/editorial';

?

Another option is to do what DCR does (in about 80% of places) and use the Type suffix, like

import { SpecialType, PillarType } from '@guardian/libs';

the folder thing is an option but i'd rather avoid restructuring libs in order to accommodate importing ambiguous exports from types. e.g. we could potentially end up exporting two things with the same from different places which is even more ambiguity.

how do you feel about these instead?

import type { ArticleTheme, ArticleFormat } from '@guardian/libs'; import { ArticlePillar, ArticleSpecial, ArticleDesign, ArticleDisplay } from '@guardian/libs';

it has the advantage of not implying that only certain teams should them and ArticleDesign.MatchReport reads clearly to me?

another option: if this is still unacceptably verbose, perhaps that's a sign these things shouldn't live in here?

I think I'd actually prefer EditorialDesign to ArticleDesign, because Article is a Design, so you'll end up with ArticleDesign.Article 🙈.

another option: if this is still unacceptably verbose, perhaps that's a sign these things shouldn't live in here?

That's reasonable. It's possible that they don't. They're specifically Editorial concepts and this is meant to be a dept-wide repo. They're only being put here for temporary convenience until we can get them into CAPI. Maybe instead we should just not deprecate @guardian/types until that migration to CAPI has finished?

ah right that's interesting. so it sounds as though something like EditorialContentDesign.Article is probably an accurate, unambiguous name (or something like it), but the amount of context you need to pack in to that name perhaps means it's not generic enough to live in here (whereas living in CAPI provides that context implicitly)

I mean, really, Article should be News or Standard

It probably should be Standard, you're right Oliver 🙄.

I don't think it should be News because a) it's not always used for news pieces, and b) News is a pillar 🙂.

Just to clarify, is everyone happy for these values to change to

import type { ArticleTheme, ArticleFormat } from '@guardian/libs'; import { ArticlePillar, ArticleSpecial, ArticleDesign, ArticleDisplay } from '@guardian/libs';

and Design.Article to become ArticleDesign.Standard or do we close this PR and figure out a better place for Format to live?

I'm happy with ArticleTheme etc. 👍

Maybe the move from Article > Standard could be a separate PR?

But also 👍 for Standard

github-actions · 2021-07-13T12:50:56Z

🎉 This PR is included in version 2.6.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

jamie-lynch requested review from gtrufitt and a team as code owners June 23, 2021 14:07

jamie-lynch changed the title ~~migrate format from types~~ feat: migrate format from types Jun 23, 2021

Jamie Lynch and others added 2 commits June 23, 2021 15:41

migrate format from types

0862b2d

chore(docs): update TOC

a8a0ada

jamie-lynch force-pushed the migrate-format branch from e42530e to a8a0ada Compare June 23, 2021 14:41

Add tests for format enums

153c5bb

jamie-lynch commented Jun 23, 2021

View reviewed changes

jamie-lynch mentioned this pull request Jun 23, 2021

feat: migrate role from types #222

Merged

JamieB-gu reviewed Jun 24, 2021

View reviewed changes

src/format.ts Show resolved Hide resolved

src/format.README.md Outdated Show resolved Hide resolved

Jamie Lynch and others added 2 commits June 24, 2021 13:59

Fix heading order in README

3949bf7

Co-authored-by: Jamie B <[email protected]>

add Design.Obituary

a77c8a6

JamieB-gu previously approved these changes Jun 24, 2021

View reviewed changes

Merge branch 'main' into migrate-format

26649e8

sndrs previously approved these changes Jun 28, 2021

View reviewed changes

Merge branch 'main' into migrate-format

9b491ec

jamie-lynch dismissed stale reviews from sndrs and JamieB-gu via 9b491ec June 28, 2021 16:08

chore(docs): update TOC

fd35329

jamie-lynch changed the title ~~feat: migrate format from types~~ feat: migrate format from @guardian/types Jun 28, 2021

nothing

286e9ef

sndrs reviewed Jun 28, 2021

View reviewed changes

add Article prefix to format exports

6a0a6cd

jamie-lynch requested review from sndrs, oliverlloyd and JamieB-gu July 8, 2021 12:56

Merge branch 'main' into migrate-format

de02427

JamieB-gu approved these changes Jul 12, 2021

View reviewed changes

jamie-lynch merged commit 654a80f into main Jul 13, 2021

jamie-lynch deleted the migrate-format branch July 13, 2021 12:48

github-actions bot added the released label Jul 13, 2021

jamie-lynch mentioned this pull request Jul 13, 2021

refactor!: rename ArticleDesign.Article to ArticleDesign.Standard #244

Merged

JamieB-gu mentioned this pull request Jul 29, 2021

Un-deprecate guardian/types#161

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate format from `@guardian/types` #220

feat: migrate format from `@guardian/types` #220

jamie-lynch commented Jun 23, 2021 •

edited

Loading

jamie-lynch commented Jun 23, 2021

coveralls commented Jun 23, 2021 •

edited

Loading

jamie-lynch Jun 23, 2021 •

edited

Loading

JamieB-gu Jun 24, 2021

mxdvl Jun 24, 2021

buck06191 Jun 24, 2021

JamieB-gu left a comment

sndrs Jun 28, 2021

JamieB-gu Jun 28, 2021

oliverlloyd Jun 28, 2021

sndrs Jun 29, 2021

JamieB-gu Jun 29, 2021

sndrs Jun 29, 2021

JamieB-gu Jun 29, 2021

jamie-lynch Jul 5, 2021

oliverlloyd Jul 5, 2021

oliverlloyd Jul 5, 2021

github-actions bot commented Jul 13, 2021

feat: migrate format from @guardian/types #220

feat: migrate format from @guardian/types #220

Conversation

jamie-lynch commented Jun 23, 2021 • edited Loading

What does this change?

Why?

jamie-lynch commented Jun 23, 2021

coveralls commented Jun 23, 2021 • edited Loading

jamie-lynch Jun 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JamieB-gu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 13, 2021

feat: migrate format from `@guardian/types` #220

feat: migrate format from `@guardian/types` #220

jamie-lynch commented Jun 23, 2021 •

edited

Loading

coveralls commented Jun 23, 2021 •

edited

Loading

jamie-lynch Jun 23, 2021 •

edited

Loading