This repository has been archived by the owner on Sep 1, 2022. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: migrate format from
@guardian/types
#220feat: migrate format from
@guardian/types
#220Changes from 3 commits
0862b2d
a8a0ada
153c5bb
3949bf7
a77c8a6
26649e8
9b491ec
fd35329
286e9ef
6a0a6cd
de02427
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we need to think about these names here. i know they have big significance in editorial, but examples like this:
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 upimport { 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
?
Another option is to do what DCR does (in about 80% of places) and use the
Type
suffix, likeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd actually prefer
EditorialDesign
toArticleDesign
, becauseArticle
is aDesign
, so you'll end up withArticleDesign.Article
🙈.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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, is everyone happy for these values to change to
and
Design.Article
to becomeArticleDesign.Standard
or do we close this PR and figure out a better place forFormat
to live?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with
ArticleTheme
etc. 👍Maybe the move from
Article
>Standard
could be a separate PR?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But also 👍 for
Standard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 otherenum
s 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems Jest will always report enums as uncovered, unless maybe you use these enums somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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