• 
      

    Adding helpful hints for rbt commands

    Review Request #11405 — Created Jan. 24, 2021 and discarded

    Information

    RBTools
    master

    Reviewers

    RBTools currently must scan the filesystem when a user does not specify a
    repository name or repository type which is very slow. In order to optimize
    RBTools, we will log hints that recommends setting repository name or type
    when it has not been specified. An option --no-hints is added so
    users can disable hints if desired.

    I ran ./tests/runtests.py rbtools/ and have 263 tests passing and have done
    manual testing on different rbtools commands to ensure that the --no-hints
    option is working and is also presented as an option to the user when they
    type rbt [command] --help.

    Summary ID Author
    reverting .reviewboardrc change
    c186fdb4eae575704451e84f3063309628bb8f17 anahita-m
    Description From Last Updated

    This looks pretty good, but I have a thought. We have these hints right now, but we may want more …

    chipx86chipx86

    Looks like .reviewboardrc is part of your change, but it shouldn't be. Can you revert that part of your change?

    chipx86chipx86

    Typo in the description: "RBTools current" -> "RBTools currently". Also in the description, "when it already has not been specified." …

    mike_conleymike_conley

    E501 line too long (93 > 79 characters)

    reviewbotreviewbot

    E501 line too long (89 > 79 characters)

    reviewbotreviewbot

    E122 continuation line missing indentation or outdented

    reviewbotreviewbot

    E502 the backslash is redundant between brackets

    reviewbotreviewbot

    This should be "True" instead of "TRUE"

    daviddavid

    Let's say "RBTools commands may print helpful hints"

    daviddavid

    "be provided" suggests to me that one could specify a value. Perhaps say "Hints can also be disabled by passing …

    daviddavid

    Setting the repository type and name each enable different optimizations, and for best results users should have both in their …

    daviddavid

    This was already mixed up, but can we alphabetize this by key while we're editing it? That'll make it easier …

    daviddavid

    Space at the end of the string after the .

    ryankangryankang

    These log messages don't make it entirely clear why the user should set these or what the consequences are of …

    daviddavid

    E501 line too long (83 > 79 characters)

    reviewbotreviewbot

    This is wrapping way prematurely. The docs should have room to breathe. The lines get to go up to 79 …

    chipx86chipx86

    Blank line between statements and blocks, or between two separate blocks.

    chipx86chipx86

    Christian's comment still applies - this .reviewboardrc file shouldn't be in here. See https://github.com/k88hudson/git-flight-rules#i-want-to-remove-a-file-from-the-previous-commit on how to clean that up.

    mike_conleymike_conley

    Typo: -no-hints -> --no-hints.

    mike_conleymike_conley

    Typo: "will speed up will speed up" -> "will speed up"

    mike_conleymike_conley
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    amohapatra
    Review request changed
    Commits:
    Summary ID Author
    [WIP] Adding helpful hints for rbt commands
    RBTools current must scan the filesystem when a user does not specify a repository name or repository type which is very slow. In order to optimize RBTools, we will log hints that recommends setting repository name or type when it already has not been specified. An option --no-hints is added so users can disable hints if desired.
    695f5a8911c1f124721f0b317328eda12b25942c anahita-m
    [WIP] Adding helpful hints for rbt commands
    RBTools current must scan the filesystem when a user does not specify a repository name or repository type which is very slow. In order to optimize RBTools, we will log hints that recommends setting repository name or type when it already has not been specified. An option --no-hints is added so users can disable hints if desired.
    c1c7bafee8df1144f518bb923da571dd5ef00af7 anahita-m

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    amohapatra
    amohapatra
    amohapatra
    amohapatra
    amohapatra
    amohapatra
    amohapatra
    Review request changed
    Commits:
    Summary ID Author
    [WIP] Adding helpful hints for rbt commands
    RBTools current must scan the filesystem when a user does not specify a repository name or repository type which is very slow. In order to optimize RBTools, we will log hints that recommends setting repository name or type when it already has not been specified. An option --no-hints is added so users can disable hints if desired.
    179b12c155e0ae001061f54c7cb189dff0297bef anahita-m
    [WIP] Adding helpful hints for rbt commands
    RBTools current must scan the filesystem when a user does not specify a repository name or repository type which is very slow. In order to optimize RBTools, we will log hints that recommends setting repository name or type when it already has not been specified. An option --no-hints is added so users can disable hints if desired. I ran ./tests/runtests.py rbtools/ and have 263 tests passing and have done manual testing on different rbtools commands to ensure that the --no-hints option is working and is also presented as a global option to the user when they type .
    a785c2f723e13218fd6c1ca66bfc2d59db5c09b3 anahita-m

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    amohapatra
    david
    1. 
        
    2. Show all issues

      This should be "True" instead of "TRUE"

    3. Show all issues

      Let's say "RBTools commands may print helpful hints"

    4. Show all issues

      "be provided" suggests to me that one could specify a value. Perhaps say "Hints can also be disabled by passing ..."?

    5. rbtools/commands/__init__.py (Diff revision 7)
       
       
       
      Show all issues

      Setting the repository type and name each enable different optimizations, and for best results users should have both in their config file. We should probably have separate hints.

      Also, FYI, when we're wrapping a conditional like this we prefer using parentheses instead of the continuation character:

      if (self.options.repository_name is None and
          self.options.repository_type is None):
      
    6. rbtools/utils/commands.py (Diff revision 7)
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      This was already mixed up, but can we alphabetize this by key while we're editing it? That'll make it easier to read.

    7. 
        
    amohapatra
    amohapatra
    ryankang
    1. 
        
    2. rbtools/commands/__init__.py (Diff revision 9)
       
       
      Show all issues

      Space at the end of the string after the .

    3. 
        
    amohapatra
    amohapatra
    amohapatra
    david
    1. 
        
    2. rbtools/commands/__init__.py (Diff revision 11)
       
       
       
       
       
       
       
      Show all issues

      These log messages don't make it entirely clear why the user should set these or what the consequences are of not doing so. How about something like:

      RBTools is auto-detecting the repository type, which can be slow. Adding REPOSITORY_TYPE to your .reviewboardrc will speed up rbt commands. Set ENABLE_HINTS=False to suppress this message.

      RBTools is attempting to discover the matching repository in the Review Board server. Adding REPOSITORY_NAME to your .reviewboardrc will speed up rbt commands. Set ENABLE_HINTS=False to suppress this message.

      Looking at those, I'm reconsidering my suggesting to combine them into a single message, but maybe there's a good way to word a single message without it being too excessively long.

    3. 
        
    amohapatra
    Review request changed
    Commits:
    Summary ID Author
    Adding helpful hints for rbt commands
    RBTools current must scan the filesystem when a user does not specify a repository name or repository type which is very slow. In order to optimize RBTools, we will log hints that recommends setting repository name or type when it already has not been specified. An option --no-hints is added so users can disable hints if desired. I ran ./tests/runtests.py rbtools/ and have 263 tests passing and have done manual testing on different rbtools commands to ensure that the --no-hints option is working and is also presented as a global option to the user when they type .
    2e8c116e97939f101887bae421b622c3ef27dff1 anahita-m
    Adding helpful hints for rbt commands
    RBTools current must scan the filesystem when a user does not specify a repository name or repository type which is very slow. In order to optimize RBTools, we will log hints that recommends setting repository name or type when it already has not been specified. An option --no-hints is added so users can disable hints if desired. I ran ./tests/runtests.py rbtools/ and have 263 tests passing and have done manual testing on different rbtools commands to ensure that the --no-hints option is working and is also presented as a global option to the user when they type .
    f7272c460fe0ab71b61f5c46d0bf704f59a5e239 anahita-m

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    amohapatra
    chipx86
    1. 
        
    2. Show all issues

      This looks pretty good, but I have a thought. We have these hints right now, but we may want more in the future, and we probably don't want each hint to decide what the output looks like (e.g., the info on suppressing hints being tacked onto every string).

      How about defining a new show_hint method on the Command class that takes in a string, checks if hints are enabled, and conditionally outputs it.

      That would clean up the code using it now, and make it very easy to start adding new hints down the road.

    3. Show all issues

      Looks like .reviewboardrc is part of your change, but it shouldn't be. Can you revert that part of your change?

    4. rbtools/commands/__init__.py (Diff revision 13)
       
       
       
       
      Show all issues

      This is wrapping way prematurely. The docs should have room to breathe. The lines get to go up to 79 characters.

    5. rbtools/commands/__init__.py (Diff revision 13)
       
       
       
      Show all issues

      Blank line between statements and blocks, or between two separate blocks.

    6. 
        
    amohapatra
    mike_conley
    1. Thanks, Anahita! Just a few comments below.

    2. Show all issues

      Typo in the description: "RBTools current" -> "RBTools currently".

      Also in the description, "when it already has not been specified." is kind of awkward. Maybe this can just be "when it has not been specified".

    3. .reviewboardrc (Diff revision 14)
       
       
       
       
       
       
      Show all issues

      Christian's comment still applies - this .reviewboardrc file shouldn't be in here. See https://github.com/k88hudson/git-flight-rules#i-want-to-remove-a-file-from-the-previous-commit on how to clean that up.

    4. docs/rbtools/rbt/configuration/users.rst (Diff revision 14)
       
       
      Show all issues

      Typo: -no-hints -> --no-hints.

    5. rbtools/commands/__init__.py (Diff revision 14)
       
       
       
      Show all issues

      Typo: "will speed up will speed up" -> "will speed up"

    6. 
        
    amohapatra
    david
    Review request changed
    Status:
    Discarded