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

Made the b command more useful #418

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

camsn0w
Copy link
Contributor

@camsn0w camsn0w commented Jan 11, 2022

Addresses #237 by making it possible to use a regex instead of an exact filename.

@camsn0w camsn0w marked this pull request as draft January 11, 2022 01:04
@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #418 (b90ae5b) into master (d0f2601) will increase coverage by 0.03%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #418      +/-   ##
==========================================
+ Coverage   56.96%   56.99%   +0.03%     
==========================================
  Files          55       55              
  Lines       10680    10683       +3     
==========================================
+ Hits         6084     6089       +5     
+ Misses       4164     4163       -1     
+ Partials      432      431       -1     
Impacted Files Coverage Δ
ecmd.go 64.31% <57.14%> (+0.35%) ⬆️

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 d0f2601...b90ae5b. Read the comment docs.

Copy link
Owner

@rjkroege rjkroege left a comment

Choose a reason for hiding this comment

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

w.r.t. e286f0a

  • ecmd.go:160: toOEB and its invokees seem complicated for what it does. (This was true before your change. It's just odd.)
  • ecmd.go:164: once your changes to make b more useful are done, we will probably discover that curtext is getting set incorrectly in numerous places. Sigh.

This patch seems fine except for (as you noted) the absence of tests. Please add some tests? Do you want to address the discussion in #237 in this PR or land this first and then address that?

@camsn0w camsn0w marked this pull request as ready for review January 27, 2022 04:13
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