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

Rename 'Plugin' & 'Config' to 'Hook' and 'HookConfig' #241

Merged
merged 15 commits into from Feb 7, 2019

Conversation

Projects
None yet
2 participants
@charlespierce
Copy link
Member

charlespierce commented Jan 16, 2019

Implements the changes discussed in notion-cli/rfcs#25

Changes

  • Combined config and plugin modules into a single modules named hook.
  • Updated Inventory::fetch and Distro construction to reference the hooks and use them to resolve the URLs necessary for locating the latest version, index and specific distro download.
  • Added tests to confirm the behavior of the different hook variants (Prefix and Template).

Notes

  • The node hooks for latest and index are currently treated the same way, they download the index, which is then searched to find the appropriate version. This will probably need to be documented, or if Node has an URL we can hit in the same way as the Yarn latest-version endpoint to determine the latest version, we can migrate to that.
  • This will likely have conflicts with #234, so once that is merged I will update to resolve conflicts.
  • There is a slight issue with this and the Node Index Caching: The cache is just a single file, so there is no way currently for it to distinguish between sources. That means if the index is cached and the hook changes so that it should be downloading from somewhere else, the cache will still be used. Filed #242 to address a more robust cache.
@charlespierce

This comment has been minimized.

Copy link
Member Author

charlespierce commented Jan 18, 2019

Resolved Merge Conflicts

@charlespierce charlespierce requested review from dherman and mikrostew Feb 1, 2019

@dherman
Copy link
Member

dherman left a comment

This looks great. Just a few small suggestions.

Show resolved Hide resolved crates/notion-core/src/distro/node.rs Outdated
Show resolved Hide resolved crates/notion-core/src/distro/yarn.rs Outdated
Show resolved Hide resolved crates/notion-core/src/hook/tool.rs Outdated
Show resolved Hide resolved crates/notion-core/src/hook/tool.rs Outdated
Show resolved Hide resolved crates/notion-core/src/hook/tool.rs Outdated
Show resolved Hide resolved crates/notion-core/src/hook/tool.rs Outdated
Show resolved Hide resolved crates/notion-core/src/hook/tool.rs
Show resolved Hide resolved crates/notion-core/src/hook/tool.rs
@dherman

dherman approved these changes Feb 7, 2019

Copy link
Member

dherman left a comment

Exciting work!

@dherman dherman merged commit b4aefb1 into notion-cli:master Feb 7, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@charlespierce charlespierce deleted the charlespierce:rename_config_to_hooks branch Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment