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

Refactor command line interface #14

Merged
merged 2 commits into from
Jan 31, 2017

Conversation

hlokavarapu
Copy link
Contributor

I have rewired the code to call the new command line interface @tjesser-ucdavis-edu .

@hlokavarapu hlokavarapu changed the title Call new CLI commands Refactor command line interface Jan 9, 2017
import argparse
import os
import pprint
import re

import config_handling
Copy link
Contributor

@tjesser-ucdavis-edu tjesser-ucdavis-edu Jan 10, 2017

Choose a reason for hiding this comment

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

Might as well remove this import too. utils/load_config.py is possibly a working replacement.

@@ -54,7 +54,7 @@ def validate_config_stream(stream):
return valid_config


def load_config(proj_dir):
def load_config(proj_dir="."):
Copy link
Contributor

@tjesser-ucdavis-edu tjesser-ucdavis-edu Jan 10, 2017

Choose a reason for hiding this comment

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

This doesn't need a default, does it? Just update the call in software_dmv.py.

Copy link
Contributor Author

@hlokavarapu hlokavarapu Jan 11, 2017

Choose a reason for hiding this comment

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

How then will a user specify a project path, proj_dir, different from the current working directory?

In other words, is there a CLI option to override this variable?

Thereby, I could call utils.load_config(path) vs utils.load_config(".")

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand what you're saying here. It sounds like you're arguing for removing the proj_dir argument entirely, since it probably won't be anything other than current directory.

My point was that, in terms of this utility function, assuming "." as a default makes more sense in software_dmv.py than in here.

HEADER_IN_FIRST_N_LINES = 20
HEADER_SIGNAL_STRING = "Copyright"

import license_handling

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the new CLI doesn't use this file, changing it shouldn't be in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, however, the file program_commands/check_command imports userfiles_handling which then wraps back to import userfiles_handling

For example,


Traceback (most recent call last):
  File "software_dmv.py", line 8, in <module>
    import program_commands.check_command as check_command
  File "/home/harsha_lv/CIG_codes/cig_tools/Licensing_Program/program_commands/check_command.py", line 4, in <module>
    import userfiles_handling
  File "/home/harsha_lv/CIG_codes/cig_tools/Licensing_Program/userfiles_handling.py", line 10, in <module>
    import license_handling
  File "/home/harsha_lv/CIG_codes/cig_tools/Licensing_Program/license_handling.py", line 9, in <module>
    import config_handling
  File "/home/harsha_lv/CIG_codes/cig_tools/Licensing_Program/config_handling.py", line 28, in <module>
    "maximum": userfiles_handling.HEADER_IN_FIRST_N_LINES,
AttributeError: module 'userfiles_handling' has no attribute 'HEADER_IN_FIRST_N_LINES'

The following functions are defined in userfiles_handling and are called in check_command module:

  • userfile_handling.sanitize_path
  • userfile_handling.userfiles_iter
  • userfiles_handling.remove_ignored_userfiles
  • userfiles_handling.commentfmt_userfiles_pairing
  • userfiles_handling.match_header_in_file

Copy link
Contributor

Choose a reason for hiding this comment

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

Darn. Okay, it's probably better to save removing this dependency for a different PR then.

@hlokavarapu
Copy link
Contributor Author

This PR will resolve issue #8 once merged.

@tjesser-ucdavis-edu tjesser-ucdavis-edu merged commit a4b0e6d into geodynamics:master Jan 31, 2017
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.

None yet

2 participants