Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Cleanup jobs flag parsing #610

Merged
merged 1 commit into from
Jun 3, 2020
Merged

Cleanup jobs flag parsing #610

merged 1 commit into from
Jun 3, 2020

Conversation

timonegk
Copy link
Member

@timonegk timonegk commented Jun 3, 2020

Summary of the changes:

  • change log space location flag from -l to -L to avoid conflicts (closes Short flag for log space not working #567)
  • clean up regular expressions that were not really readable and add explanatory comments to the new ones
  • add two additional small tests regarding the job flags
  • add -l to parser to be shown on --help
  • specify type of -l, -j and -p options for more useful error messages

@timonegk
Copy link
Member Author

timonegk commented Jun 3, 2020

Also #516 and #555 were already fixed in #573 and can be closed.

Summary of the changes:

 * change log space location flag from -l to -L to avoid conflicts (closes #567)
 * clean up regular expressions that were not really readable and add explanatory comments to the new ones
 * add two additional small tests regarding the job flags
 * add `-l` to parser to be shown on `--help`
 * specify type of `-l`, `-j` and `-p` options for more useful error messages
@mikepurvis
Copy link
Member

This looks reasonable as well. Would be great to have some tests covering this, but it's a good start.

@mikepurvis mikepurvis merged commit 9a3cc31 into catkin:master Jun 3, 2020
@timonegk
Copy link
Member Author

timonegk commented Jun 3, 2020

Unit tests for both functions exist in tests/unit/test_job_flag_regex.py.

@timonegk timonegk deleted the fix/regex branch June 3, 2020 20:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Short flag for log space not working
2 participants