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.

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

add @guardian/libs code #63

Merged
merged 4 commits into from
Sep 1, 2022
Merged

add @guardian/libs code #63

merged 4 commits into from
Sep 1, 2022

Conversation

sndrs
Copy link
Member

@sndrs sndrs commented Aug 31, 2022

@sndrs sndrs requested a review from a team as a code owner August 31, 2022 15:43
@changeset-bot
Copy link

changeset-bot bot commented Aug 31, 2022

⚠️ No Changeset found

Latest commit: d39ea68

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the 📦 npm Affects a @guardian package on NPM label Aug 31, 2022
Comment on lines +14 to +15
return cookieValue
.replace('|', ';')

Check failure

Code scanning / CodeQL

Incomplete string escaping or encoding

This replaces only the first occurrence of '|'.
@sndrs
Copy link
Member Author

sndrs commented Aug 31, 2022

@mxdvl are you ok with the changes in ede6453? (hard to spot them because the file is also added)

@@ -104,24 +104,3 @@ If you are using this library with TypeScript, make sure you are using at least
This package uses `ES2020`.

If your target environment does not support that, make sure you transpile this package when bundling your application.

## Development
Copy link
Contributor

Choose a reason for hiding this comment

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

are these sections now covered by a more general monorepo readme? maybe we could link to where that is, if it exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, this readme is the info gets displayed on npm for example, so it's still needed here

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry — I was referring to the ones you removed

Copy link
Member Author

Choose a reason for hiding this comment

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

no that's something we need to address, but probably beyond the scope of this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

okay, is it recorded somewhere that we need to address this? (on trello or similar)

Copy link
Contributor

@mxdvl mxdvl left a comment

Choose a reason for hiding this comment

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

Thanks for updating the logger script!

Is it a problem at all that we have now lost git history? I'm assuming as we were squashing and merging, the change log is roughly equivalent.

@sndrs
Copy link
Member Author

sndrs commented Aug 31, 2022

Is it a problem at all that we have now lost git history?

we'll always have the history in the original repo – do you think we need to manage bringing in the history for the code that moves here?

the conversations around this so far have tended towards concluding that history is useful to a point, but maybe not worth the noise it introduces in a migration like this (esp where the history starts again, but the earlier stages can be found in the old home)

@sndrs
Copy link
Member Author

sndrs commented Sep 1, 2022

Is it a problem at all that we have now lost git history?

maybe it would be useful to add a link to the original repo in the merge commit – that way if someone is diving into the history of these files, at least they'd eventually reach that commit that would redirect them to the old repo and it's history

@mxdvl
Copy link
Contributor

mxdvl commented Sep 1, 2022

maybe it would be useful to add a link to the original repo in the merge commit – that way if someone is diving into the history of these files, at least they'd eventually reach that commit that would redirect them to the old repo and it's history

I think that would be very beneficial, along with a small explanation of the decision in the commit message and a link to the change log.

Contrary to many other repos, I would not expect people to rely on blame to delve into historical code, so a singular commit makes sense. Things that make this library unique are:

  • 100% coverage
  • extensive documentation
  • strictest typescript checks
  • versions and change log
  • exports independent modules
  • not driven by external business requirements

@OllysCoding
Copy link
Contributor

along with a small explanation of the decision in the commit message and a link to the change log.

I'd agree with this - it's worth remembering as well that if you were bringing in the previous history, you end up with mis-matched PR #'s on the old commits, as they same PRs don't exist in the new repository.

As long as when navigating back to the end of the history in the new repo, you can get a link to the old repo & basic information on what's happened, this is likely better than having a lot of old history which doesn't link up well to the new repo

@sndrs sndrs merged commit 6fe0985 into main Sep 1, 2022
@sndrs sndrs deleted the add-libs-content branch September 1, 2022 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 npm Affects a @guardian package on NPM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants