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

Remove duplicate key #1000

Merged
merged 2 commits into from
Nov 9, 2023
Merged

Remove duplicate key #1000

merged 2 commits into from
Nov 9, 2023

Conversation

AdrianaCeric
Copy link
Contributor

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • All tests and CI builds passing

Description

I'm not sure how I originally found this. I'm also not sure if this has impact but it's probably best to remove?

Copy link
Contributor

@philip-cline philip-cline left a comment

Choose a reason for hiding this comment

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

Good catch, it's just flow typing so seems good.

@@ -238,8 +237,6 @@ export type CustomFile = {
useDuringServe: boolean
}

// TODO: Remove this eslint rule once https://github.com/babel/babel-eslint/pull/584
// is merged in.
/* eslint-disable no-use-before-define */
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to remove this too since the PR above is merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we have an old version of flow since I still go the pre-commit lint error when I removed this

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok we should change the TODO then to be updating eslint rather than waiting for the PR to be merged. I think the TODO is still relevant as long as the error is thrown

@@ -575,9 +572,6 @@ export type Label = {
projectId: string,
user?: string
}

// TODO: Remove this eslint rule once https://github.com/babel/babel-eslint/pull/584
// is merged in.
/* eslint-enable no-use-before-define */
Copy link
Contributor

Choose a reason for hiding this comment

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

Likewise

Copy link
Contributor

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

This is a really fitting 1000th pr for datatools. Agree with Phil on removing those comments until we upgrade/replace mastarm

@AdrianaCeric
Copy link
Contributor Author

woah happy 1000

Copy link
Contributor

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

It's beautiful! Happy 1000

@philip-cline
Copy link
Contributor

Happy 1000 everyone. Time to merge the documentation now

@AdrianaCeric AdrianaCeric merged commit 5994927 into dev Nov 9, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants