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

Issue 59 harmonize commands #62

Merged
merged 8 commits into from
Nov 17, 2022
Merged

Issue 59 harmonize commands #62

merged 8 commits into from
Nov 17, 2022

Conversation

cybersec-code
Copy link
Contributor

Changes needed to restructure the bin/tagpack-tool main commands. Three files were modified to comply with the suggestions of flake8 (bin/tagpack-tool, tagpack/tagpack.py, tagpack/tagstore.py)

Copy link
Member

@mdragaschnig mdragaschnig left a comment

Choose a reason for hiding this comment

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

I set up a fresh tagstore and tested

  • config create/show
  • taxonomy show/insert
  • confidence show/ingest
  • tagpack validate/insert
  • tagstore housekeeping
  • quality calculate/show

All looks good.

A suggestion: we still have 'tagpack insert' and 'confidence ingest' etc, might use either ingest or insert everywhere.

@cybersec-code
Copy link
Contributor Author

Did you use the version of the last commit? confidence command shouldn't exist anymore (now it's integrated with taxonomy)

@mdragaschnig
Copy link
Member

Did you use the version of the last commit? confidence command shouldn't exist anymore (now it's integrated with taxonomy)

No, I had a version before that commit.

Just updated and checked

  • taxonomy show/insert confidence

Looking good 👍

Copy link
Member

@soad003 soad003 left a comment

Choose a reason for hiding this comment

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

I also set up a new Db and ran through the instructions in the README.md. My main issues are all related to the documentation.

Documentation

  • tagpack-tool validate tests/testfiles/ should be tagpack-tool tagpack validate
  • same for insert
  • tagstore init is also not mentioned in the README
  • Ingest confidence scores (according to the code, this section is not accurate anymore)
  • It also took me some time (thanks for the help @mdragaschnig) to figure out that I have to recreate my local config.yaml to add the confidence.csv to the db again. Without that, all tagpack ingests fail (some fk errors). I think this should go in the README as well.

Nits

  • It looks like to shorten strings (i guess because of flake8 line to long errors) some new string concatenations were introduced in the code. There are better ways to split strings into multiple lines in python see
  • Command behavior is not always consistent. Sometimes commands have a "default" function like taxonomy (list) and quality (calculate) which automatically runs if invoked. Others do not like tagstore.
  • Commands like tagstore show the error message no action was requested if invoked without sub command. I think it would be more helpful to show the help output of the sub-command instead (if possible). In general, I would prefer the help output instead of default behavior.

I guess the nits are open for discussion, but the changes to the README are necessary before a merge.

tagpack/tagpack.py Show resolved Hide resolved
Copy link
Member

@soad003 soad003 left a comment

Choose a reason for hiding this comment

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

Thanks for the additions. I think this covers all my documentation issues.

@soad003 soad003 merged commit 7dd6c7b into develop Nov 17, 2022
@soad003 soad003 deleted the issue-59_harmonize_commands branch November 17, 2022 17:52
@cybersec-code
Copy link
Contributor Author

Thanks for the detailed comments, maybe we can open another issue to address the consistency of the commands (i.e. the default actions).

The string concatenations were introduced to comply with the style guides recommended by flake8, and I think it's just about coding preferences. The problem with the approach in the link you suggest is that splitting strings that way will introduce undesired tabs (\t) to the resulting string (e.g. when used inside a function). I prefer the concatenation of strings either like:

s = "Very long"
s += " message"

or:

s = "Very long" \
    " message"

But again, I think it's just about preferences, feel free to change them back.

@soad003
Copy link
Member

soad003 commented Nov 18, 2022

Thank you for the work you put in to harmonize the tools interface.

Sure sounds good. Lets open another issue to harmonize the default actions.

Regarding the string concatenations, sure it's a preference. I was just wondering why you have chosen this particular explicit concatenation style, its verbose in my opinion. Regarding the tabs, I should have mentioned the answer I meant (see comment of Stevoisiak). My intention was to use:

s =  ("Very long"
      " message")

Python does automatic string literal concatination ref. I think it's a pretty clean and quick way to split strings in python.

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.

3 participants