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

Get twiglet and db #18

Merged
merged 9 commits into from
May 28, 2019
Merged

Get twiglet and db #18

merged 9 commits into from
May 28, 2019

Conversation

moon-tea
Copy link
Contributor

Resolved most of the todos.

Copy link
Contributor

@lizziesz lizziesz left a comment

Choose a reason for hiding this comment

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

I like the refactor of getTwigletInfoAndDB. Nice work

@jeffbergman
Copy link
Contributor

jeffbergman commented May 24, 2019

  1. You have a commit called replacing trailing commas, yet there are a lot of places where there seems to be no trailing comma. I really like the trailing comma, and I don't agree to removing it.
  2. How did that pass linting? Did you change linting rules or was that rule never there?

@moon-tea
Copy link
Contributor Author

moon-tea commented May 24, 2019

  1. You have a commit called replacing trailing commas, yet there are a lot of places where there seems to be no trailing comma. I really like the trailing comma, and I don't agree to removing it.
  2. How did that pass linting? Did you change linting rules or was that rule never there?
  1. I will fix those, I thought it would be fixed automatically. I personally really like the trailing comma as well.
  2. The rule is not enforced in the eslint file, I can add it.

@moon-tea
Copy link
Contributor Author

  1. You have a commit called replacing trailing commas, yet there are a lot of places where there seems to be no trailing comma. I really like the trailing comma, and I don't agree to removing it.
  2. How did that pass linting? Did you change linting rules or was that rule never there?
  1. I will fix those, I thought it would be fixed automatically. I personally really like the trailing comma as well.
  2. The rule is not enforced in the eslint file, I can add it.

Okay, the rule is now enforced and it changed a lot of files. We now have the trailing comma everywhere it can be.

@moon-tea moon-tea requested a review from jeffbergman May 24, 2019 21:27
@lizziesz
Copy link
Contributor

I don't necessarily agree with changing linting rules as part of this branch (although I prefer the trailing commas too, and seems like it must not have been consistently enforced?) Since we all prefer trailing commas (and we're the current dev team on Twig) I think this change is okay, but we should avoid changing any more linting rules as part of this PR and focus on the task at hand, which is updating all the packages and making sure tests pass.

@jeffbergman
Copy link
Contributor

@lizziesz I agree no further linting rule changes. When I initially reviewed the pr, I only saw places where there had been a trailing comma that was removed in the new code - so I wanted them put back. I couldn't see places where a trailing comma had not existed previously, if there were any, because they weren't changes. So I don't know whether any inconsistency had existed before.

Copy link
Contributor

@jeffbergman jeffbergman left a comment

Choose a reason for hiding this comment

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

Nothing to add that I haven't said already.

@moon-tea moon-tea merged commit a135c64 into updating-all-packages May 28, 2019
@moon-tea moon-tea deleted the get-twiglet-and-db branch May 28, 2019 18:36
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