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.

Sign up for GitHub

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

Jump to bottom

Fix the solidus executable and make it rely on just Thor #49

Merged
merged 3 commits into from
Jan 10, 2020
Merged

Fix the solidus executable and make it rely on just Thor #49

merged 3 commits into from
Jan 10, 2020

Conversation

elia
Copy link
Member

@elia elia commented Jan 3, 2020

Fixes #47

Summary

  • Only use Thor (instead of mixing with optparse) and let it print the help message
  • Define a base SolidusCLI command that will be the base for extension
  • Define an extension command with a generate subcommand, which is also the default
  • Cleanup the implementation and make it work with both relative and absolute paths
  • add bin/solidus and bin/rake for easy development and less typing

Checklist

  • I have structured the commits for clarity and conciseness.
  • I have added relevant automated tests for this change.
  • I have added an entry to the changelog for this change.

@elia elia added bug Describes or fixes a bug. documentation Improves the documentation. labels Jan 3, 2020
@elia elia self-assigned this Jan 3, 2020
@elia elia force-pushed the elia/update-current-extension-with-the-executable branch from e6da2bf to b204200 Compare January 3, 2020 17:18
aldesantis
aldesantis requested changes Jan 3, 2020
View reviewed changes
lib/solidus_dev_support/solidus_cli.rb Outdated
require 'thor'
require 'solidus_dev_support/extension'

class SolidusDevSupport::SolidusCLI < Thor
Copy link
Member

Choose a reason for hiding this comment

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

Couple of things:

  1. Is there a reason for using class SolidusDevSupport::SolidusCLI instead of nesting class SolidusCLI within a module SolidusDevSupport definition?
  2. SoliudsCLI is not compatible with Zeitwerk, which would expect it to be defined at solidus_c_l_i.rb. How about renaming it to SolidusCli? It's a no-brainer and prevents any potential problems.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do that by default because it keeps everything on a single level of indentation and avoids some constant lookup situations. But I'll expand it and rename the class to SolidusCommand.

In the future I think it's something that should be extracted (to solidus-cli?) and added as a dependency of the solidus gem, but that's another story.

@codecov
Copy link

codecov bot commented Jan 3, 2020 •
edited
Loading

Codecov Report

Merging #49 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #49   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          11     11           
=====================================
  Hits           11     11

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bfc11d...588d0b5. Read the comment docs.

@elia elia force-pushed the elia/update-current-extension-with-the-executable branch from b204200 to f0b8fdc Compare January 3, 2020 17:48
@elia elia requested a review from aldesantis January 3, 2020 17:48
@elia elia force-pushed the elia/update-current-extension-with-the-executable branch from f0b8fdc to 1d67dda Compare January 3, 2020 17:51
@aldesantis
Copy link
Member

aldesantis commented Jan 10, 2020 •
edited
Loading

@elia please add a changelog entry for the path bugfix too! 🙏

@elia
Use only Thor instead of mixing it with optparse
7c13dd6 
- Let Thor print the help message
- Define a base SolidusCLI command that will be the base for `extension`
- Define an `extension` command with a generate subcommand, which is also the default
- Cleanup the implementation and make it work with both relative and absolute paths
@elia
Add a binstub for the rake command
01ce713 
Will make life easier while developing.
@elia
Add a changelog entry for the executable updates
588d0b5
@elia elia force-pushed the elia/update-current-extension-with-the-executable branch from 1d67dda to 588d0b5 Compare January 10, 2020 13:39
@elia elia merged commit 2737abd into master Jan 10, 2020
@elia elia deleted the elia/update-current-extension-with-the-executable branch January 10, 2020 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aldesantis aldesantis aldesantis approved these changes

@vassalloandrea vassalloandrea Awaiting requested review from vassalloandrea

Assignees

@elia elia

Labels
bug Describes or fixes a bug. documentation Improves the documentation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow users to easily update the extension to new standards
2 participants
@elia @aldesantis