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

Merge cli repo for better dependency management #139 #169

Merged
merged 236 commits into from
Mar 18, 2018

Conversation

acjh
Copy link
Contributor

@acjh acjh commented Mar 17, 2018

Resolves #139

I did git merge cli --allow-unrelated-histories to preserve histories from both repos.
As such, there are 200+ commits. Here are the newest commits that are specific to this PR:

  • df3a62b Move core library into lib/markbind
  • 7402e2a Update eslintignore for lib/markdown-it/*
  • 59830af Merge branch 'cli'
  • 83b373c Restore eslint-plugin-lodash
  • 9962681 Use local markbind (overwritten by force-push)
  • c36f595 Use local markbind

  1. To test this, cd to the markbind folder, pull this branch, and then run (sudo) npm link.
  2. We can publish markbind-cli from the root, and publish markbind core from ./lib/markbind.
  3. Caveat: When running npm version, tagging is now committed for cli but not the core library.
    • This means that if you push that commit, the markbind-cli tag will be added to this repo.
    • Maybe create a version-specific branch that is only pushed to MarkBind/markbind-cli repo.
    • Committing and tagging for the core library has to be done manually (smallest issue here).
    • ⚠ Care must be taken that maintainers with write access do not "Push all tags" in SourceTree:
      screenshot 2018-03-17 23 32 39

@acjh acjh requested a review from Gisonrg March 17, 2018 15:42
@acjh
Copy link
Contributor Author

acjh commented Mar 17, 2018

It should be easy to merge changes from MarkBind/markbind-cli when existing PRs there are merged.

@acjh
Copy link
Contributor Author

acjh commented Mar 17, 2018

If this PR is merged, I can release [email protected] from this merged repo tomorrow (or Monday).

@acjh
Copy link
Contributor Author

acjh commented Mar 18, 2018

  1. Caveat: When running npm version, tagging is now committed for cli but not the core library.
    • ...

Or we could sync the release versions, and then we can use the same Milestone and tags for both!

@nicholaschuayunzhi
Copy link
Contributor

To clarify

To test this, cd to the markbind folder, pull this branch, and then run (sudo) npm link

If we're using local markbind in c36f595 we don't have to run the $ npm link command right?

@acjh
Copy link
Contributor Author

acjh commented Mar 18, 2018

npm link binds the markbind command and the global node_modules/markbind-cli to that folder.

You should have run it before, but that was the unmerged markbind-cli folder:
https://github.com/MarkBind/markbind-cli/wiki/Developer-Guide#development

  1. To make sure you are using the cloned CLI program in your own terminal/console, in the cloned CLI repo, run

    $ npm link
    

@Gisonrg
Copy link
Contributor

Gisonrg commented Mar 18, 2018

Thanks @acjh for the efforts!!! The merge is not easy 👏

npm version has a command flag which disallow auto commit tag, we could enforce that by automation (could use npm scripts) and well written release guide.

Also we need to migrate the Wiki page, which works similar to change a git remote address (Wikis are just a repo).

Copy link
Contributor

@Gisonrg Gisonrg left a comment

Choose a reason for hiding this comment

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

Migration looks good to me. But make sure we test:

  1. All functions are working, including publish.
  2. Try publish the markbind-core in the merged repo, after that try install it (using a new node project) to see if it could correctly identify the package.

Also might need to mark the markbind-cli repo as deprecated (remove the code and update the README).

package.json Outdated
},
"repository": {
"type": "git",
"url": "https://github.com/MarkBind/markbind-cli.git"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should change this to MarkBind/markbind

@acjh
Copy link
Contributor Author

acjh commented Mar 18, 2018

Published [email protected] and tested like this:

$ cd ~/Desktop
$ mkdir tmp
$ cd tmp
$ npm init
$ # Press enter 10 times.
$ npm install [email protected]
$ echo "const MarkBind = require('markbind');" > index.js
$ echo "console.log(new MarkBind());" >> index.js
$ node index.js

Output:

Parser {
  _options: {},
  _onError: [Function: bound consoleCall],
  _fileCache: {},
  dynamicIncludeSrc: [],
  staticIncludeSrc: [],
  boilerplateIncludeSrc: [],
  missingIncludeSrc: [] }

acjh and others added 4 commits March 18, 2018 23:10
@acjh
Copy link
Contributor Author

acjh commented Mar 18, 2018

Merge worked fine.

@acjh acjh added this to the v1.6.1 milestone Mar 18, 2018
@acjh acjh merged commit ec8119f into MarkBind:master Mar 18, 2018
@acjh
Copy link
Contributor Author

acjh commented Mar 18, 2018

Also we need to migrate the Wiki page, which works similar to change a git remote address (Wikis are just a repo).

I was thinking of going straight to #135. Easier to track future changes and PRs.

Also might need to mark the markbind-cli repo as deprecated (remove the code and update the README).

I updated the README. I'll remove the code when the remaining PRs are closed.

@acjh
Copy link
Contributor Author

acjh commented Mar 18, 2018

@Gisonrg Can you activate Travis on this repo? 🙈

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.

5 participants