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

Removes unused dependencies #212

Merged
merged 1 commit into from Jun 25, 2019
Merged

Removes unused dependencies #212

merged 1 commit into from Jun 25, 2019

Conversation

artem-zakharchenko
Copy link
Contributor

This pull request reviews and uninstalls unused dependencies from gavel. No breaking changes are introduced.

GitHub

@artem-zakharchenko
Copy link
Contributor Author

Dependencies analyzis

Including the insights about other dependencies that may be easy to remove.

clone

Used only at headers-json-example validator to deep copy the original result from JsonSchema and mutate it with a new message:

const resultCopy = clone(result, false);

caseless

Used only in tv4-to-headers-message to reference expectedHeaders headers keys in a caseless way.

const expected = caseless(expectedHeaders).get(headerName);

@honzajavorek
Copy link
Contributor

I thought commander is used for the CLI?

var cmd = require('commander');

Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

Except of commander it looks good :) Thanks!

@artem-zakharchenko
Copy link
Contributor Author

Good catch.

That's the only place I found it being used:

cmd.version('0.0.1');

Does it only sets a version, or are there some side-effects in place that I'm not seeing?

@honzajavorek
Copy link
Contributor

Does it only sets a version, or are there some side-effects in place that I'm not seeing?

That's out of scope of my memory. We'd need to dig into the ancient scrolls in the attic to learn about that.

@artem-zakharchenko
Copy link
Contributor Author

artem-zakharchenko commented Jun 25, 2019

By examining the source code of commander, I can conclude that it's a util library to build CLI modules. It doesn't seem to do anything but setting the version of the program, as there is explicit syntax like cmd.option(), cmd.action(), etc.

I'm not familiar with commander, but we don't see to use it at the moment to its full extent. At the same time, it's an approved dependency, so, perhaps, we should keep it?

The functionality of commander looks very similar to yargs that I'm used to. I think we can build a good CLI design with commander, or at least can try to do that.

@honzajavorek
Copy link
Contributor

So it looks like we don't need it 😄 But I think it is okay to keep it, not to add ourselves work. Up to you. I'm more concerned about the CI passing with non-executable Gavel CLI. I wonder if the CLI tests even run.

@artem-zakharchenko
Copy link
Contributor Author

commander module reverted and remains the dependency of gavel module. Should be ready to be merged.

@honzajavorek honzajavorek merged commit 3c0247e into master Jun 25, 2019
@artem-zakharchenko artem-zakharchenko deleted the deps-clean-up branch June 25, 2019 10:23
@ApiaryBot
Copy link
Collaborator

🎉 This PR is included in version 6.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clean up dependencies
3 participants