Change cookie, configuration and cache files to use appdir path.

Review Request #11423 — Created Jan. 31, 2021 and updated

qianxi
RBTools
master
4859
rbtools

We need to unify the locations that store cookie, config and cache files for the later releases.
Copy any legacy cookie files to the new appdir path and create new cookie file at the new appdir path if no legacy cookie files exist.

Write a test for generate configuration file in appdir path and pass it.

Write a test for create_cookie_jar and pass it.

Summary
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
Fix extra space before newline. Fix too long lines.
Remove spaces in new line.
Add unit test to create_cookie_jar.
Fix trailing white space, over-indented problems.
Fix test_http_request.py
Fix create_cookie_jar function.
Fix setup_repo.py.
Fix appdirs.py and remove unnecessary test from test_setup_repo.py.
Fix filesystem.py
Fix coding style problems.
Fix more coding style problems.
Fix extra space.
Fix style problems
Finish unit test for create_cookie_jar function
Fix coding style problems and remove unused libraries and variables.
Fix coding style problems.
Description From Last Updated

Your summary and description need some work. See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea for our guide on how to write these.

daviddavid

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

F841 local variable 'original_cookie' is assigned to but never used

reviewbotreviewbot

This kind of comment isn't particularly useful because it can get out of date quickly. It's better to give a ...

daviddavid

I'm thinking if it might be a lot simpler to have a list of legacy paths, which would include the ...

daviddavid

We don't use strings-as-comments like this.

daviddavid

Comments should start with capital letters and end in periods.

daviddavid

These imports are all out of order. It wasn't correct before but now it's worse. There should be three sections: ...

daviddavid

Comments should start with capital letters and end in periods.

daviddavid

This should probably say appdirs_path rather than XDG. XDG applies to Linux but appdirs supports windows/mac too.

daviddavid

Comments should start with capital letters and end in periods.

daviddavid

Comments should start with capital letters and end in periods.

daviddavid

Comments should start with capital letters and end in periods.

daviddavid

I'm not really sure what this means. We're not doing anything with repos.

daviddavid

This new line should be removed.

daviddavid

In this case, we don't want to change it. setup-repo generates a .reviewboardrc file that users should add to their ...

daviddavid

Why this change? appdirs is a third-party module that we just happen to bundle.

daviddavid

This shouldn't be needed.

daviddavid

This should happen adjancent to the check for the one in the home path. The ones from walk_parents need to ...

daviddavid

E501 line too long (80 > 79 characters)

reviewbotreviewbot

F401 'rbtools.utils.appdirs.user_config_dir' imported but unused

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

appdirs should come before encoding in this list.

daviddavid

There should be a space between the argument name and the type. Inside the parens, this should say (unicode, optional) ...

daviddavid

This should be: Returns: tuple: A 2-tuple with the following items: 1. A :py:class:`http.cookiejar.MozillaCookieJar` object that can load and save ...

daviddavid

For these, let's use a loop. If you recall our last meeting, someone mentioned Python's for/else syntax, which is perfect ...

daviddavid

As mentioned in my other comment, you need to be using appdir_cookie_file here (cookie_file will never have been defined if ...

daviddavid

Let's reword this a bit: If we didn't find any existing cookie files, create a new one.

daviddavid

We may not be able to assume that the appdirs directory exists already (unlike $HOME, which we can). Let's do ...

daviddavid

This test is using files from the user's system. It therefore seems like different setups might yield different results. The ...

daviddavid

This still needs to be reverted. rbt setup-repo should write to a .reviewboardrc file in the current working directory, not ...

daviddavid

There should still be two blank lines here.

daviddavid

Capitalize the first letter of this comment.

daviddavid

Please use single quotes here instead of double.

daviddavid

E999 SyntaxError: invalid syntax

reviewbotreviewbot

F401 'io' imported but unused

reviewbotreviewbot

F401 'mock' imported but unused

reviewbotreviewbot

F401 'sys' imported but unused

reviewbotreviewbot

F841 local variable 'cookie_file' is assigned to but never used

reviewbotreviewbot

F401 'rbtools.utils.appdirs.user_config_dir' imported but unused

reviewbotreviewbot

This extra blank line should be removed, because there are still some (extra) imports just below this. There are correctly ...

daviddavid

We should create temporary directories and use kgb to override get_home_path and user_cache_dir to return the paths to the temporary ...

daviddavid

This file should not be part of this change.

daviddavid

Please remove this line.

daviddavid

Please add a blank line between these two.

daviddavid

Let's update this comment to say "Check for a config file in the home directory", since the appdirs file is ...

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

flake8

qianxi
Review request changed

Change Summary:

Fix coding style problems.

Description:

~  

[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.

  ~

[WIP] Edit cookie creation and path checking. Pass the test for XDG config file.

Commits:

Summary
-
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
-
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
+
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
+
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
+
Fix extra space before newline. Fix too long lines.

Bugs:

+4859

Diff:

Revision 2 (+214 -64)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

qianxi
qianxi
Review request changed

Change Summary:

Add a unit test to create_cookie_jar and pass the test.

Summary:

-[WIP] Add XDG path to config file. Add XDG path to cookie file. Caches already in the right path so no changes
+Add XDG path to config file. Add XDG path to cookie file. Caches already in the right path so no changes

Description:

~  

[WIP] Edit cookie creation and path checking. Pass the test for XDG config file.

  ~

Edit the default path for creating cookie, configuration files. Alse checked the path for storing cache, since it is using XDG path already so no need to change.

  + We need to unify the locations that store cookie, config and cache files for the later releases and we still want the users with older releases can stick to the original locations that store these files.

Commits:

Summary
-
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
-
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
-
Fix extra space before newline. Fix too long lines.
-
Remove spaces in new line.
+
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
+
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
+
Fix extra space before newline. Fix too long lines.
+
Remove spaces in new line.
+
Add unit test to create_cookie_jar.

Diff:

Revision 4 (+322 -64)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

qianxi
qianxi
david
  1. 
      
  2. rbtools/api/request.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     

    This kind of comment isn't particularly useful because it can get out of date quickly. It's better to give a higher-level overview of what file locations we use and which we prefer.

    While you're in here, mind updating this method docstring to include "Args" and "Returns" sections?

  3. rbtools/api/request.py (Diff revision 5)
     
     
     

    I'm thinking if it might be a lot simpler to have a list of legacy paths, which would include the old post-review-cookies file and the home path (assuming that the appdirs path is not just $HOME/.rbtools-cookies). If the appdirs path doesn't exist, we can then scan through the legacy paths and move their content if they exist.

  4. rbtools/api/request.py (Diff revision 5)
     
     
     
     
     

    We don't use strings-as-comments like this.

  5. rbtools/api/request.py (Diff revision 5)
     
     

    Comments should start with capital letters and end in periods.

  6. rbtools/api/tests/test_http_request.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     

    These imports are all out of order. It wasn't correct before but now it's worse.

    There should be three sections: standard library, third-party modules, and then anything from rbtools. Within each section, we keep things alphabetized:

    import os
    
    import six
    from kgb import SpyAgency
    from six.moves.urllib.parse import parse_qsl, urlparse
    
    from rbtools.api.request import HttpRequest, create_cookie_jar
    from rbtools.testing import TestCase
    from rbtools.utils.appdirs import user_cache_dir
    from rbtools.utils.filestystem import get_home_path
    
    1. I see, I've almost never paid any attentions to the order before. From now on I'll handle it carefully.

  7. rbtools/api/tests/test_http_request.py (Diff revision 5)
     
     
     

    Comments should start with capital letters and end in periods.

  8. rbtools/api/tests/test_http_request.py (Diff revision 5)
     
     

    This should probably say appdirs_path rather than XDG. XDG applies to Linux but appdirs supports windows/mac too.

  9. rbtools/api/tests/test_http_request.py (Diff revision 5)
     
     
     

    Comments should start with capital letters and end in periods.

  10. rbtools/api/tests/test_http_request.py (Diff revision 5)
     
     
     

    Comments should start with capital letters and end in periods.

    1. Okay, I'll pay more attention to this problem next time.

  11. rbtools/api/tests/test_http_request.py (Diff revision 5)
     
     

    Comments should start with capital letters and end in periods.

  12. rbtools/commands/setup_repo.py (Diff revision 5)
     
     
     
     

    I'm not really sure what this means. We're not doing anything with repos.

  13. rbtools/commands/setup_repo.py (Diff revision 5)
     
     

    This new line should be removed.

  14. rbtools/commands/setup_repo.py (Diff revision 5)
     
     

    In this case, we don't want to change it. setup-repo generates a .reviewboardrc file that users should add to their repository. It's separate from the .reviewboardrc in their home directory. That actually probably means most/all of your changes to generate_config_file should be reverted too.

  15. rbtools/utils/appdirs.py (Diff revision 5)
     
     

    Why this change? appdirs is a third-party module that we just happen to bundle.

    1. Well, I was testing what should be the appdirs path for those files, I changed it just for testing. I'll change it back.

  16. rbtools/utils/filesystem.py (Diff revision 5)
     
     

    This shouldn't be needed.

  17. rbtools/utils/filesystem.py (Diff revision 5)
     
     
     
     
     
     
     
     

    This should happen adjancent to the check for the one in the home path. The ones from walk_parents need to happen first.

    We might want to call this appdirs_path too. XDG applies to linux but appdirs will do the right thing for windows/mac too.

    1. I see, I'll change all misused XDG name to appdirs related name.

  18. 
      
qianxi
Review request changed

Change Summary:

Fix all problems described in David's review comments.

Summary:

-[WIP] Add XDG path to config file. Add XDG path to cookie file. Caches already in the right path so no changes
+Add appdir path to config file. Add appdir path to cookie file. Caches already in the right path so no changes

Description:

~  

Edit the default path for creating cookie, configuration files. Alse checked the path for storing cache, since it is using XDG path already so no need to change.

  ~

Edit the default path for creating cookie, configuration files. Alse checked the path for storing cache, since it is using appdir path already so no need to change.

    We need to unify the locations that store cookie, config and cache files for the later releases and we still want the users with older releases can stick with the original locations that store these files.

Testing Done:

~  

Write a test for generate configuration file in XDG path and pass it.

  ~

Write a test for generate configuration file in appdir path and pass it.

   
   

Use the XDG config path as input for generate_config_file method, then check whether there's a config file generated in that path.

Commits:

Summary
-
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
-
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
-
Fix extra space before newline. Fix too long lines.
-
Remove spaces in new line.
-
Add unit test to create_cookie_jar.
-
Fix trailing white space, over-indented problems.
+
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
+
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
+
Fix extra space before newline. Fix too long lines.
+
Remove spaces in new line.
+
Add unit test to create_cookie_jar.
+
Fix trailing white space, over-indented problems.
+
Fix test_http_request.py
+
Fix create_cookie_jar function.
+
Fix setup_repo.py.
+
Fix appdirs.py and remove unnecessary test from test_setup_repo.py.
+
Fix filesystem.py
+
Fix coding style problems.

Diff:

Revision 6 (+421 -211)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

qianxi
Review request changed

Change Summary:

Fix coding style problems.

Commits:

Summary
-
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
-
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
-
Fix extra space before newline. Fix too long lines.
-
Remove spaces in new line.
-
Add unit test to create_cookie_jar.
-
Fix trailing white space, over-indented problems.
-
Fix test_http_request.py
-
Fix create_cookie_jar function.
-
Fix setup_repo.py.
-
Fix appdirs.py and remove unnecessary test from test_setup_repo.py.
-
Fix filesystem.py
-
Fix coding style problems.
+
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
+
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
+
Fix extra space before newline. Fix too long lines.
+
Remove spaces in new line.
+
Add unit test to create_cookie_jar.
+
Fix trailing white space, over-indented problems.
+
Fix test_http_request.py
+
Fix create_cookie_jar function.
+
Fix setup_repo.py.
+
Fix appdirs.py and remove unnecessary test from test_setup_repo.py.
+
Fix filesystem.py
+
Fix coding style problems.
+
Fix more coding style problems.

Diff:

Revision 7 (+424 -214)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

qianxi
qianxi
david
  1. Since you took the "WIP" tag off of the summary, I'm going to start being more nitpicky from here on out.

  2. Your summary and description need some work. See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea for our guide on how to write these.

  3. rbtools/api/request.py (Diff revision 8)
     
     
     
     

    appdirs should come before encoding in this list.

  4. rbtools/api/request.py (Diff revision 8)
     
     

    There should be a space between the argument name and the type. Inside the parens, this should say (unicode, optional)

    This also needs a colon at the end of the line.

  5. rbtools/api/request.py (Diff revision 8)
     
     
     
     
     
     

    This should be:

    Returns:
        tuple:
        A 2-tuple with the following items:
    
        1. A :py:class:`http.cookiejar.MozillaCookieJar` object
           that can load and save cookies to disk.
        2. The path to the cookie file.
    
  6. rbtools/api/request.py (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
     
     

    For these, let's use a loop. If you recall our last meeting, someone mentioned Python's for/else syntax, which is perfect here:

    for path in legacy_list:
        if os.path.isfile(path):
            try:
                shutil.copyfile(path, appdir_cookie_file)
                os.chmod(appdir_cookie_file, 0o600)
            except IOError as e:
                logging.warning(...)
    else:
        try:
            open(appdir_cookie_file, 'w').close()
            ...
    
  7. rbtools/api/request.py (Diff revision 8)
     
     

    As mentioned in my other comment, you need to be using appdir_cookie_file here (cookie_file will never have been defined if we get to this part of the code).

    Given this and my comment about your unit tests, your existing test cases aren't going through this code path. We should both fix up the tests to generate situations whereby this code will be executed and verified, and also do manual testing for this situation.

  8. rbtools/api/request.py (Diff revision 8)
     
     
     

    Let's reword this a bit:

    If we didn't find any existing cookie files, create a new one.

  9. rbtools/api/request.py (Diff revision 8)
     
     

    We may not be able to assume that the appdirs directory exists already (unlike $HOME, which we can). Let's do this first:

    try:
        os.makedirs(os.path.dirname(appdir_cookie_file), 0o700)
    except OSError:
        pass
    
  10. rbtools/api/tests/test_http_request.py (Diff revision 8)
     
     
     
     
     

    This test is using files from the user's system. It therefore seems like different setups might yield different results.

    The tests should be completely isolated from the system. We should be creating new directories with test cookie file data, and then using kgb to override appdirs methods to point the code to the fake data.

  11. rbtools/commands/setup_repo.py (Diff revision 8)
     
     
     

    This still needs to be reverted. rbt setup-repo should write to a .reviewboardrc file in the current working directory, not the user's personal config dir.

  12. rbtools/utils/filesystem.py (Diff revision 8)
     
     

    There should still be two blank lines here.

  13. rbtools/utils/filesystem.py (Diff revision 8)
     
     

    Capitalize the first letter of this comment.

  14. rbtools/utils/filesystem.py (Diff revision 8)
     
     

    Please use single quotes here instead of double.

  15. 
      
qianxi
Review request changed

Change Summary:

Finish unit test for create_cookie_jar function. Fix coding style problems.

Summary:

-Add appdir path to config file. Add appdir path to cookie file. Caches already in the right path so no changes
+Change cookie, configuration and cache files to use appdir path.

Description:

~  

Edit the default path for creating cookie, configuration files. Alse checked the path for storing cache, since it is using appdir path already so no need to change.

~   We need to unify the locations that store cookie, config and cache files for the later releases and we still want the users with older releases can stick with the original locations that store these files.

  ~

We need to unify the locations that store cookie, config and cache files for the later releases.

  ~ Copy any legacy cookie files to the new appdir path and create new cookie file at the new appdir path if no legacy cookie files exist.

Commits:

Summary
-
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
-
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
-
Fix extra space before newline. Fix too long lines.
-
Remove spaces in new line.
-
Add unit test to create_cookie_jar.
-
Fix trailing white space, over-indented problems.
-
Fix test_http_request.py
-
Fix create_cookie_jar function.
-
Fix setup_repo.py.
-
Fix appdirs.py and remove unnecessary test from test_setup_repo.py.
-
Fix filesystem.py
-
Fix coding style problems.
-
Fix more coding style problems.
-
Fix extra space.
+
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
+
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
+
Fix extra space before newline. Fix too long lines.
+
Remove spaces in new line.
+
Add unit test to create_cookie_jar.
+
Fix trailing white space, over-indented problems.
+
Fix test_http_request.py
+
Fix create_cookie_jar function.
+
Fix setup_repo.py.
+
Fix appdirs.py and remove unnecessary test from test_setup_repo.py.
+
Fix filesystem.py
+
Fix coding style problems.
+
Fix more coding style problems.
+
Fix extra space.
+
Fix style problems
+
Finish unit test for create_cookie_jar function

Diff:

Revision 9 (+823 -279)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

qianxi
david
  1. 
      
  2. rbtools/api/request.py (Diff revision 10)
     
     

    This extra blank line should be removed, because there are still some (extra) imports just below this. There are correctly two blank lines between all the imports and the RBTOOLS_COOKIE_FILE definition.

  3. rbtools/api/tests/test_http_request.py (Diff revision 10)
     
     
     
     

    We should create temporary directories and use kgb to override get_home_path and user_cache_dir to return the paths to the temporary directories. Right now it seems likely that this unit test will end up manipulating stuff in $HOME and ~/.config, which we really don't want.

    1. Actually I asked Chris about this problem. I'm currently inheriting from RBTestBase and it will set the $HOME temporarily to a path like "/var/folders/fr/yc_d5r_x16d38xjg9ydfpp480000gn/T/rbtools.12ezj6b_/". I think it should be safer now.

  4. rbtools/commands/api.py (Diff revision 10)
     
     

    This file should not be part of this change.

  5. rbtools/commands/setup_repo.py (Diff revision 10)
     
     

    Please remove this line.

  6. rbtools/utils/filesystem.py (Diff revision 10)
     
     
     

    Please add a blank line between these two.

  7. rbtools/utils/filesystem.py (Diff revision 10)
     
     

    Let's update this comment to say "Check for a config file in the home directory", since the appdirs file is also the "user's own config"

  8. 
      
qianxi
Review request changed

Change Summary:

Fix coding style problems. And the class of test_create_cookie_jar is inheriting from RBTestBase now, it will set the $HOME to a path like /var/folders/fr/yc_d5r_x16d38xjg9ydfpp480000gn/T/rbtools.12ezj6b_/ temporarily.

Commits:

Summary
-
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
-
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
-
Fix extra space before newline. Fix too long lines.
-
Remove spaces in new line.
-
Add unit test to create_cookie_jar.
-
Fix trailing white space, over-indented problems.
-
Fix test_http_request.py
-
Fix create_cookie_jar function.
-
Fix setup_repo.py.
-
Fix appdirs.py and remove unnecessary test from test_setup_repo.py.
-
Fix filesystem.py
-
Fix coding style problems.
-
Fix more coding style problems.
-
Fix extra space.
-
Fix style problems
-
Finish unit test for create_cookie_jar function
-
Fix coding style problems and remove unused libraries and variables.
+
[WIP] Finish the draft code for the cookie and config. Write a unit test for the config generation Cache already using XDG path so I don't see any path needs to change so far.
+
[WIP] Edit cookie creating and path checking. Pass the test for XDG config file.
+
Fix extra space before newline. Fix too long lines.
+
Remove spaces in new line.
+
Add unit test to create_cookie_jar.
+
Fix trailing white space, over-indented problems.
+
Fix test_http_request.py
+
Fix create_cookie_jar function.
+
Fix setup_repo.py.
+
Fix appdirs.py and remove unnecessary test from test_setup_repo.py.
+
Fix filesystem.py
+
Fix coding style problems.
+
Fix more coding style problems.
+
Fix extra space.
+
Fix style problems
+
Finish unit test for create_cookie_jar function
+
Fix coding style problems and remove unused libraries and variables.
+
Fix coding style problems.

Diff:

Revision 11 (+662 -450)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...