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

Lower case command arguments user and repo #85

Merged
merged 1 commit into from
Aug 17, 2020
Merged

Lower case command arguments user and repo #85

merged 1 commit into from
Aug 17, 2020

Conversation

BoniLindsley
Copy link
Contributor

Fix #84: Ensure import and export find the same issue directory.

I have added lower casing to import/export functions that parse command line arguments. I think I have caught all of them. Four considerations had come to mind:

  1. The create_issue function is used internally in a for loop in two other functions. So this adds redundant lower casing in these loops. This is unlikely to be a bottleneck though.
  2. I did not add a test case to test.sh. I can have a go at it, but it will take me a bit of time to understand the test script.
  3. This commit is likely to clash with git issue import can fetch issues from origin with no arguments #82. The pull request in there may also need to consider letter cases?
  4. I decided against lower casing $provider. All definitions of the variable from user input are preceded by an explicit comparison to github and gitlab, making the lower casing entirely redundant. Moving the comparison to after the lower casing is possible to allow, for example, git issue import GitLab <...>. Please let me know if that is desirable.

Fix #84: Ensure `import` and `export` find the same issue directory.
@FrazerClews
Copy link
Contributor

FrazerClews commented Aug 17, 2020

  1. This commit is likely to clash with git issue import can fetch issues from origin with no arguments #82. The pull request in there may also need to consider letter cases?

Thanks for the heads up. Mine is ready for review for the functionality, but testing ideally needs implementing, but still going through how to implement it. I don't mind if #82 or #85 is merged first, I could add the tests in another patch if that's easier

As for if #82 will need to consider it, I don't think so since it fetches the information from git remote get-url ${remote}|origin. It may need considering for ${remote} and will need rebasing

@dspinellis dspinellis merged commit c0e9f80 into dspinellis:master Aug 17, 2020
@dspinellis
Copy link
Owner

Thank you!

@dspinellis
Copy link
Owner

Can you please fix the shellcheck warnings reported in the Travis CI build?

@BoniLindsley BoniLindsley deleted the fix-84-letter-case-of-user-repo branch August 17, 2020 22:07
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.

Inconsistent argument letter case handling
3 participants