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

Base config and file names. #160

Closed
wants to merge 2 commits into from
Closed

Conversation

rob3000
Copy link
Member

@rob3000 rob3000 commented Aug 14, 2020

Hi Guys,

So after doing: nodefluent/node-sinek#154 there was a lot to review. So this time i thought it would be easier to split it up. This is the first PR to move to typescript.

  • Renamed files .js -> .ts
  • Added eslint
  • Fixed es6 import and exports.

Next steps:

  • Fix actual types.
  • Fix tests to work with typescript.

Apologies @wtrocki if i'm stepping on toes with this PR.

@rob3000 rob3000 requested review from wtrocki and krystianity August 14, 2020 08:14
@wtrocki
Copy link
Member

wtrocki commented Aug 14, 2020

When renaming git will keep original history.
The way this changes were done it creates new files instead of renames. Also not the way I would suggest this to approach it. The best way IMHO will be to move one file at the time - with useJs: true.
Done migration many times.
I have spent couple hours fixing tests and typings. You are free to take over this issue and contribute something meaningful, no problem, but -100 on approach in this PR.

@rob3000
Copy link
Member Author

rob3000 commented Aug 14, 2020

okay. will close this and approach this differently.

@rob3000 rob3000 closed this Aug 14, 2020
@wtrocki
Copy link
Member

wtrocki commented Aug 14, 2020

Perfect! Let's collaborate on this no worries

@rob3000
Copy link
Member Author

rob3000 commented Aug 14, 2020

What's the best way to collaborate on this?

@wtrocki
Copy link
Member

wtrocki commented Aug 14, 2020

If you want to start fresh feel free to do so. There will be a lot of files to move so we can jump into those etc. Back from holidays next week

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.

2 participants