-
Notifications
You must be signed in to change notification settings - Fork 66
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
git issue import can fetch issues from origin with no arguments #82
base: master
Are you sure you want to change the base?
git issue import can fetch issues from origin with no arguments #82
Conversation
lib/git-issue/import-export.sh
Outdated
|
||
get_provider() | ||
{ | ||
provider=$(echo "$1" | grep -oP "(?<=@).*?(?=\.)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function and the ones' below can simply be echo | grep
(No need for assignment and second echo). Even better, use expr match or sed, rather than grep whose use for this purpose isn't common.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify:
get_provider()
{
echo "$1" | grep -oP "(?<=@).*?(?=\.)"
}
or better
get_provider()
{
expr match "$1" 'suitable RE here'
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Using expr match
gives the warning https://github.com/koalaman/shellcheck/wiki/SC2003 . Will sed
or another command like cut
do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. It seems that using :
will not trigger the warning. Using sed is also fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made some changes using sed
and even got it working with https
URLs. Do you prefer the sed
styling on line 905, or line 910 and 915? Had trouble with getting the provider
sed
working with the extended regex styling
Thanks for the heads-up. I made a couple of improvement suggestions. |
5e96621
to
ca7fb82
Compare
ca7fb82
to
816b034
Compare
9fe4107
to
0fb8fdc
Compare
bb14620
to
02a6cbf
Compare
bbc33f1
to
016fbb9
Compare
Think this PR is ready for review and potential merge if you're happy with it? I looked into incorporating the testing, but since the test suite import stage imports from a separate repo, the new functionality will just import issues from this issue tracker and not work. |
016fbb9
to
3cc00c3
Compare
3cc00c3
to
f10444e
Compare
Allow git issue import to work without arguments by fetching the information from $(git remote get-url ${remote}). But also allow just the remote to be defined if 1 arguement is present while preserving the previous functionality, for repos that may have separate issue trackers in forked repos.
f10444e
to
5163845
Compare
#81