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

refactor(top-level): rewrite top level to typescript #679

Merged
merged 1 commit into from
Jul 7, 2019

Conversation

byCedric
Copy link
Member

@byCedric byCedric commented Jun 4, 2019

Description

Refactors @commitlint/top-level to TypeScript.

Motivation and Context

Part of #659.

Usage examples

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@byCedric
Copy link
Member Author

byCedric commented Jun 4, 2019

I had to upgrade top-level to v4 for some ts definitions. The main breaking changes are:

But unfortunately, this doesn't seem to work. I'm a bit puzzled about the error because the received CWD is actually undefined when I run it locally from the repo here. I think this should be the value of process.cwd() right? Do you know what might be wrong here @marionebl?

This is the actual issue:

TypeError: Could not find git root from undefined
    at /Volumes/Drive/Projects/conventional-changelog/commitlint/@commitlint/read/lib/index.js:71:20

@byCedric byCedric force-pushed the refactor/typescript-top-level branch from 75355dc to 41da5f4 Compare June 4, 2019 21:41
@bendtherules
Copy link
Contributor

bendtherules commented Jun 5, 2019

Hey @byCedric ,
From what I understood:

  1. the argument cwd does default to process.cwd(), but from the caller of toplevel function (cli -> read -> toplevel). In cli, cwd defaults to that
    cwd: {
    alias: 'd',
    default: process.cwd(),
    description: 'directory to execute in',
    type: 'string'
    },

Related: how are you actually testing/running it right now? It might be better to run the tests of @commitlint/read, which actually has some tests and will do the setup part (providing correct cwd) automatically.
@marionebl Not sure why top-level doesn't have its own tests - even though read has tests, it should probably also have its own basic test.

  1. type should probably be "folder" - because .git is usually a folder at the root of project
    const found = await up('.git', { cwd, type: 'file' });

Maybe this is why the tests are failing right now.

@byCedric
Copy link
Member Author

@bendtherules Sorry, I totally missed your comment here! Thanks for the check! I've just switched it to folder, I think I tried that one too (and possibly forgot to change it back 😅, my bad). I totally agree on adding some tests here! But for the typescript port, I tend to not add or remove anything. It should work with typescript first, right? 😄

@byCedric
Copy link
Member Author

If it keeps failing, I'll create a custom typing for the older version of find-up. Let's see if it's related to the 2 major version upgrade (although, I'm still not sure why)!

@bendtherules
Copy link
Contributor

Sure, we shouldn't add tests in this PR. Maybe later.

@byCedric
Copy link
Member Author

byCedric commented Jun 13, 2019

🙈 no way, that actually fixed it... I feel so stupid now 😅 Thanks, I'll rebase everything on master tonight to make it ready for merging. Again, thanks for your help ❤️

@byCedric byCedric force-pushed the refactor/typescript-top-level branch 2 times, most recently from 86d8997 to 810d9b5 Compare June 13, 2019 19:44
@byCedric
Copy link
Member Author

Interesting, I don't see tests failing on my machine (even when cleaned and reinstalled). So, do you have any idea @marionebl? 😄 (you are my only hope)

@byCedric byCedric force-pushed the refactor/typescript-top-level branch from 031495e to 5f4c746 Compare June 13, 2019 20:30
@marionebl
Copy link
Contributor

marionebl commented Jun 13, 2019

Interesting, I don't see tests failing on my machine (even when cleaned and reinstalled). So, do you have any idea @marionebl? 😄 (you are my only hope)

This is prettier telling you to format a file - you can execute yarn lint in the project root locally to get a list of malformed files

@byCedric
Copy link
Member Author

@marionebl The commands I executed locally were:

$ yarn
$ yarn clean
$ yarn build
$ yarn lint
$ yarn test

Locally this succeeded without issues. Even the lint command, might be something weird on my end. I'll explicitly run prettier and fix the prettier errors! Thanks for the tip

@bendtherules
Copy link
Contributor

bendtherules commented Jun 14, 2019

@byCedric Your changes in the rebased commit looks a little odd. There is no deleted or renamed files from .js to .ts. You probably didn't delete the .js files. That should be part of this PR.

This is probably not the reason for lint error, but still check with a fresh checkout of the commit (with both .js and .ts files with same filename present side by side).

@bendtherules
Copy link
Contributor

Just raised a PR for the small lint fix. #698

@byCedric
Copy link
Member Author

Ok, I think it's good now. Sorry for the delay and trouble! Going to squash it and make it ready for merger

@byCedric byCedric force-pushed the refactor/typescript-top-level branch from ecc0270 to da08e02 Compare June 17, 2019 20:27
@byCedric byCedric force-pushed the refactor/typescript-top-level branch from da08e02 to 4bc0977 Compare June 17, 2019 20:29
@marionebl marionebl merged commit 6a6a8b0 into master Jul 7, 2019
@marionebl marionebl deleted the refactor/typescript-top-level branch July 7, 2019 20:28
@marionebl marionebl mentioned this pull request Jul 15, 2019
18 tasks
@huanghai21 huanghai21 mentioned this pull request Sep 11, 2019
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants