Skip to content

Reorganize core exports#1262

Merged
ang-zeyu merged 5 commits into
MarkBind:masterfrom
acjh:reorganize-core-exports
Jun 28, 2020
Merged

Reorganize core exports#1262
ang-zeyu merged 5 commits into
MarkBind:masterfrom
acjh:reorganize-core-exports

Conversation

@acjh

@acjh acjh commented Jun 28, 2020

Copy link
Copy Markdown
Contributor

What is the purpose of this pull request? (put "X" next to an item, remove the rest)

• [x] Other, please explain: Module structure consistency and extensibility

What is the rationale for this request?

Have consistent module structure and file naming convention.

What changes did you make? (Give an overview)

  • bd33f78 Use PascalCase file name for class-only exports — to be consistent with imported name that is newed.
  • 91451a9 Create a module for errors — CyclicReferenceError is not a handler; error.js similar to constants.js.
  • 3e3d01d Export object instead of function — for consistency and extensibility.
  • ac4775f Introduce main index.js for MarkBind core package — to abstract ignoreTags and provide an entry point.

@ang-zeyu ang-zeyu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The entry point and object export standardising (is there an eslint rule for this?) looks good, just some comments:

Comment thread test/unit/parser.test.js
const Parser = require('markbind/src/parser');
const VariablePreprocessor = require('markbind/src/preprocessors/variablePreprocessor');
const Parser = require('markbind/src/Parser');
const VariablePreprocessor = require('markbind/src/preprocessors/VariablePreprocessor');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll be touching on variablePreprocessor shortly, don't mind if you want to include it here though

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Touching in what way?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

#1263, a bugfix + small directory reorganization for variable processing files. Plan to resume work on it after moving packages/markbind to minimise conflict resolution though

Comment thread test/unit/parser.test.js
Comment thread src/lib/markbind/index.js
markdownItEscapeSpecialTags.injectTags(tagsToIgnore);
}

module.exports = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a rationale for only exporting the following? (and not other things like errors)

*I'm fine with the cleanup as it is though, since we're going to move all the other things inside core and change the import / paths again

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Abstraction, since the caller (CLI) shouldn't need to know how ignoring is done nor access those modules directly.

Comment thread src/lib/markbind/index.js
const markdownItEscapeSpecialTags = require('./src/lib/markdown-it/markdown-it-escape-special-tags');
const Parser = require('./src/Parser');

function ignoreTags(tagsToIgnore) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Comment thread docs/devGuide/design.md

@ang-zeyu ang-zeyu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lgtm 👍, needs a commit message though, think we could use a merge commit here since it's all rather related.

@acjh acjh force-pushed the reorganize-core-exports branch from cee75f9 to 2eb6dda Compare June 28, 2020 08:10
@acjh

acjh commented Jun 28, 2020

Copy link
Copy Markdown
Contributor Author

Force-pushed the fixup. Can you help with the merge commit?

@ang-zeyu

Copy link
Copy Markdown
Contributor

Force-pushed the fixup. Can you help with the merge commit?

yup sure, thanks for cleaning this up!

@ang-zeyu ang-zeyu added this to the v2.15.0 milestone Jun 28, 2020
@ang-zeyu ang-zeyu merged commit 6ebbac8 into MarkBind:master Jun 28, 2020
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