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

Import gist #75

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Import gist #75

wants to merge 14 commits into from

Conversation

rajasegar
Copy link
Contributor

@rajasegar rajasegar commented Nov 18, 2019

#12

  • Add import command (global)
  • Test cases for import (global)
  • Add import command (local )
  • Test cases for import command (local )
  • Add export command (local )
  • Update README with the import command info

@rajasegar
Copy link
Contributor Author

@rwjblue Please let me know your comments whether we are going in the right direction.

1. Move api keys to .env file using dotenv
2. Added test cases for new with import url
Comment on lines 280 to 283
require('dotenv').config();
const Octokit = require('@octokit/rest');
const octokit = new Octokit({ auth: process.env.CODEMOD_CLI_API_KEY });
let codemodDir = `${process.cwd()}/${projectName}/transforms/${codemodName}`;
Copy link
Owner

Choose a reason for hiding this comment

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

Do we need this? Can we download the actual file directly (without needing an API key)?

I'm thinking like, from:

https://gist.githubusercontent.com/astexplorer/6f3bf899542dbe21e0474a2a74b2ffc9/raw/c23b658b268c2e5b644fed7f1a360433748e7f71/transform.js

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 yes, but in case of export we need an api to create gists, so I thought we can use this. What do you think?

Comment on lines 310 to 335
fs.outputFileSync(
`${codemodDir}/README.md`,
stripIndent`
# ${codemodName}\n

## Usage

\`\`\`
npx ${projectName} ${codemodName} path/of/files/ or/some**/*glob.js

# or

yarn global add ${projectName}
${projectName} ${codemodName} path/of/files/ or/some**/*glob.js
\`\`\`

## Input / Output

<!--FIXTURES_TOC_START-->
<!--FIXTURES_TOC_END-->

<!--FIXTURES_CONTENT_START-->
<!--FIXTURES_CONTENT_END-->
`,
'utf8'
);
Copy link
Owner

Choose a reason for hiding this comment

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

Can you abstract this so we don't slowly diverge the README.md contents?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, working on it.

let { codemodName } = options;

const Octokit = require('@octokit/rest');
const octokit = new Octokit({ auth: 'acabfddbefb244cb3e674fa84046a907c78f2294' });
Copy link
Owner

Choose a reason for hiding this comment

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

🤔 - Seems odd to hard code this here? Is this a mistake?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, my bad, will remove that.

Comment on lines 46 to 47
// https://api.github.com/gists/de5cff0a12c2aaf129f94b775306af6f
console.log(data);
Copy link
Owner

Choose a reason for hiding this comment

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

Just notating for removal in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.

.create({
files,
})
.then(({ data }) => {
Copy link
Owner

Choose a reason for hiding this comment

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

I think we can use async/await here instead of manual chaining.

Comment on lines 59 to 84
fs.outputFileSync(
`${codemodDir}/README.md`,
stripIndent`
# ${codemodName}\n

## Usage

\`\`\`
npx ${projectName} ${codemodName} path/of/files/ or/some**/*glob.js

# or

yarn global add ${projectName}
${projectName} ${codemodName} path/of/files/ or/some**/*glob.js
\`\`\`

## Input / Output

<!--FIXTURES_TOC_START-->
<!--FIXTURES_TOC_END-->

<!--FIXTURES_CONTENT_START-->
<!--FIXTURES_CONTENT_END-->
`,
'utf8'
);
Copy link
Owner

Choose a reason for hiding this comment

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

Lets use a shared function to generate these README.md's, I don't want them to slowly get out of sync due to the duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

@@ -0,0 +1,11641 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

I think we are using a yarn.lock, can you delete this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I got used to npm so much , I forgot to remove, will do it right away.

1. Added new lib for readme-support
2. Project readme abstracted to readme-support
3. Codemod readme abstracted to readme-support
4. Removed console.logs and api keys
1. Removed package-lock
2. Update yarn.lock
@rajasegar
Copy link
Contributor Author

@rwjblue Do you mind if I create a separate PR for export, after figuring out the issues with ast-explorer?

1. Add new with import command example in Readme
2. Clean up some comments and console.logs
3. Added error handling to new and import commands
1. Made changes for downloading transform with node's built-in
request object in new and import commands
2. Remove octokit from package.json and yarn.lock
3. Removed export command
@rajasegar rajasegar changed the title WIP Import gist Import gist Jan 28, 2020
@rwjblue
Copy link
Owner

rwjblue commented Mar 19, 2020

@rwjblue Do you mind if I create a separate PR for export, after figuring out the issues with ast-explorer?

No problem, that seems fine to me.

@rwjblue
Copy link
Owner

rwjblue commented Mar 19, 2020

I'm going to try to get CI passing again this 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