Suggest next sub ses remote

[cs7-shrey]

Description

What is this PR

• Bug fix
• Addition of a new feature
• Other

Why is this PR needed?
Solves issue #409

What does this PR do?
Allows users to search both local and remote repositories for next sub / ses suggestion

References

Issues #409

How has this PR been tested?

Manually tested on local and remote projects for next sub / ses suggestions. Testing still in progress.

Is this a breaking change?

No

Does this PR require an update to the documentation?

No

Checklist:

• The code has been tested locally
• Tests have been added to cover all new functionality
• The documentation has been updated to reflect any changes
• The code has been formatted with pre-commit

[cs7-shrey]

@JoeZiminski please review the current approach for this feature.

To test this locally, please consider creating a new project since this PR adds a new key to the persistent settings.

or you can change the persistent_settings.yaml file, you'll need to add suggest_next_sub_ses_local_only: false above name_templates:

[JoeZiminski]

Hey @cs7-shrey thanks for this, its looking great! I just had a quick look to play around with it on a new project and it looks like it's working perfectly. I will try and review ASAP.

The only thing things to add will be a test to ensure it's working going forward, and documentation. You can build the documentation locally with these instructions. Feel free to have a look through and let me know if you see anywhere it would make sense to add a note on this button, I think it will be useful to highlight it in the docs for users.

For tests, the approach would be similar to this test but instead of creating folders locally, it would create folder centrally and then check these are detected (similar to here). These tests are more extensive, in this case I think it would make sense to do something like:

1. make a sub-001 locally and a sub-001 and sub-002 centrally.
2. check with the checkbox off sub-002 is suggested
3. turn the checkbox on and check sub-003 is selected.

This will be sufficient to check that the local_only flag is passed.

Another approach is to monkeypatch the datashuttle.get_next_sub() and get_next_ses() function similar to here and just check the underlying functions are called with local_only=True. On balance, as we will have to set up a lot of the TUI just to trigger this monkeypatched function, we might as well just check the output on the TUI side also.

Thanks again for this nice addition and please let me know if you have any questions!

[JoeZiminski]

BTW I forgot to say, there is this function here which ensures persistent settings are backwards compatible. The new key can be added to it, the idea is to insert new keys that are not found in the dictionary (i.e. if they are from old versions).

[JoeZiminski]

Hey @cs7-shrey thanks a lot for this! Looking good, please see some points of discussion in the review. Let me know if you have any questions about anything.

[JoeZiminski]

Hey @cs7-shrey sorry for all the messages, just to let you know in #491 the argument "local_only" was changed to "include_central" and is now by default False (before it was default True but always set to False in the TUI.)

I will merge '491 shortly, I think for this PR it will just entail renaming the arguments and switching the bool value (as it is now reversed). The easiest way to handle this is probably to 'rebase' this PR onto the new version of main. Once #491 is merged into main (I'll send you a ping) you can do:

1. Go to your fork main branch and click the 'sync' button so that your fork main is up to date with datashutle main.

git checkout main
git pull
git checkout suggest_next_sub_ses_remote
git rebase -i main

This will start an interactive rebase (see this guide for detailed information) on running and handling conflicts. Let me know if you have any questions!

[cs7-shrey]

Cool, no problem! It will just require some minor changes.

[JoeZiminski]

Just FYI #491 is now merged!

[cs7-shrey]

Hey @JoeZiminski, I added a comment, please have a look! I think github doesn't notify about that

[cs7-shrey]

Hey @JoeZiminski I think something caused the tests to run I'm not sure why. Please disable them for now as it is still in development. Also, I had to force-push some changes as the remote branch wasn't accepting changes from local branch and a git pull to merge remote to local started showing existing changes (done on some previous PRs that were merged) as new changes in my branch in the pull request.

[adamltyson]

Hi @cs7-shrey the tests will run whenever a PR is raised. It's not a problem to just let them run, even if they're failing.

[sumana-2705]

Hello @cs7-shrey,

This problem is very common and can be quite irritating, I face it sometimes too. The issue is likely due to a divergence between the local and remote branches, possibly caused by a force-push in the repository history. When it occurs, I usually sync my forked repo (remote) and then run the following commands in local branch:

git fetch origin
git status

I guess this should solve the issue. Additionally, you shouldn't need to force-push the commits after this. Let me know if you need any further help! 😊

[cs7-shrey]

Thanks for the help @sumana-2705! It was probably due to something that went wrong during an interactive rebase I tried to resolve merge conflicts.

[cs7-shrey]

Hey @JoeZiminski, I've modified the code to show a popup on searching remote for sub/ses. Let me know what you think of the current approach.

[JoeZiminski]

Hey @cs7-shrey this looks great! Works really well, I made some small suggestions on things like naming / some minor refactoring's but nothing substantial.

We'll have to add some tests for this, I think the two things to test are:

1. That the status of the button is remembered across sessions
2. that the include_central tag is propagated down from the tui to the underlying datashuttle functions (through interface)

In contrast to my previous message, I think it would actually be easier to just monkeypatch get_next_sub and get_next_ses in the tests. here is an example of the set up for testing connect text ses / sub in the TUI. We can monkeypatch the datashuttle get_next_sub or get_next_ses, basically mocking the datashuttle-side functions and checking they are called correctly. For an example, this is done here with the mocker package. In the tests, we can click the input boxes, and check the underlying function is called correctly (with include_central=False). Then we can turn on the Suggest next... checkbox e.g. here is an example of switching the bypass validation checkbox (just an example of switching the other checkboxes)).

As for testing the setting is maintained across sessions, there is currently a full test of bypass validation here. I think we can repeat this for the new checkbox and settings. When writing this test, it might be that it is not worth the duplication and there may be an opportunity to centralise this, or maybe the tests are overkill. Feel free to say as such, as they are quite extensive.

As for the documentation, we can add a small section explaining this in the Create Folders page. However, the documentation are undergoing a big refactor so we can wait until #499 is merged before proceeding. BTW feel free to mark this ready to review! (rather than draft)

[cs7-shrey]

Hey @JoeZiminski, I made the required changes and added some tests but there's one particular test that keeps failing on the macos py3.9 runner.
It's in tests/tests_tui/test_tui_create_folders, It's in those tests that double click the input boxes for suggesting next sub/ses.
test_name_template_next_sub_or_ses_and_validation and test_get_next_sub_and_ses_no_template

The logs display some underlying textual code that's causing error but it doesn't clearly indicate what's causing that to happen. I can play hit and trial with the runners trying to guess what's causing it but it would be great if you could once run tests locally on a macos system and see if there's some important revelation.

[JoeZiminski]

Hey @cs7-shrey thanks this is excellent, I have a few very minor changes (some of the comment formatting is just for consistency with existing style). Once complete this can be merged, thanks!

I can replicate the test failure on my macOS, very strange that it is only for python version 3.9. Upgrading textual fix it, currently we pin to 1.0 so that textual updates don't unexpectedly break the release (I see they are already on 3.3). I think the best solution is for me to make a new PR upgrading to textual 3.3, and fixing any issues. Then once merged this PR can rebase on main and all tests should pass.

[JoeZiminski]

Hey @cs7-shrey sorry I forgot one thing, would it be a pain to make a test case that triggers dismiss_popup_and_show_modal_error_dialog_from_thread? It is unlikely to cause any problems but it would be good to have the automated testing check it at least once (just incase something e.g. a textual update breaks it in future)

[cs7-shrey]

Hey @JoeZiminski, thanks a lot for the textual update! I'm no longer having those unexpected errors. There was one particular error that was coming up in testing the error popup. After clearing the subject input box, a double click on session input box was throwing an error

It was because of repeated clicks being done in a short period of time due to which a click on subject box and the 1st click on sesion box were together being assumed as a double click and therefore the double click on session box couldn't be completed.

So, I added a new decorator for that purpose. Please have a look when you find time.

[JoeZiminski]

Hey @cs7-shrey this is a very elegant solution, I could replicate this bug manually, great job on the tests to find this subtle bug.

I would suggest one change so that the two existing functions can be combined. We can introduce a class that handles all kinds of click-related infomration, then if we want to extend it in future we can avoid adding more attributes to the relevant classes. It is very useful to ensure the same widget is clicked twice generally, so this is acheived here by extending your solution to the only other use case in datashuttle (directory tree). If we want to support more widgets this will require extendign this function which isn't ideal, but I think the other benefits outway the downside. What do you think?

It requires replacing all prev_click_time or prev_click_input_id with
self.click_info = ClickInfo()

from __future__ import annotations

from functools import wraps
from time import monotonic

# -----------------------------------------------------------------------------
# Double-click decorator
# -----------------------------------------------------------------------------

class ClickInfo:
    """
    A class to hold click-info to checking
    double clicks are within the time threshold
    and match the widget id.
    """
    def __init__(self):

        self.prev_click_time = 0.0
        self.prev_click_widget_id = ""


def require_double_click(func):
    """
    A decorator that calls the decorated function
    on a double click, otherwise will not do anything.

    Requires the first argument (`self` on the class) to
    have the attribute `click_info`). Any class holding a widget
    that supports double-clicking must have the attribute
    self.click_info = ClickInfo()

    The first (non-self) argument depends on the decorated function,
    which is usually widget-specific. Unfortunately, these must be
    supported on a case-by-case bases and extended when required.
    """

    @wraps(func)
    def wrapper(*args, **kwargs):
        parent_class = args[0]

        assert hasattr(parent_class, "click_info"), (
            "Decorator must be used on class method where the class as "
            "the attribute `self.click_info = ClickInfo()`."
        )

        click_time = monotonic()
        event = args[1]

        if hasattr(event, "input"):
            id = event.input.id
        elif hasattr(event, "node"):
            id = event.node.tree.id
        else:
            raise RuntimeError("The message type for the widget you are trying to"
                               "register clicks on is not supported. Add it to the decorator.")
        if (
            click_time - parent_class.click_info.prev_click_time < 0.5
            and id == parent_class.click_info.prev_click_widget_id
        ):
            parent_class.click_info.prev_click_time = click_time
            parent_class.click_info.prev_click_widget_id = id
            return func(*args, **kwargs)

        parent_class.click_info.prev_click_time = click_time
        parent_class.click_info.prev_click_widget_id = id

    return wrapper

[JoeZiminski]

Hey @cs7-shrey this looks great, just the one suggestion in the other comment and I think we're good to go

[JoeZiminski]

Brilliant!

[JoeZiminski]

Thanks @cs7-shrey! This is good to go, I made a slight change in reverting the rename from node to event because in this case, the object is actually a DirectoryTree TreeNode. This wasn't clear as the type hint was missing so have added that. Thanks again for this its a really useful addition to the package!

[cs7-shrey]

Hey @JoeZiminski I'm glad this work was useful. For the node and event confusion, I just ran the code in a debugger (to know the type of the argument), I found it to be DirectoryTree.DirectorySelected and DirectoryTree.FileSelected (for on_directory_tree_directory_selected and on_directory_tree_directory_selected respectively)

I also found this example in the textual source code. My bad, I should have added a type hint there.

[JoeZiminski]

Hey @cs7-shrey no you are completely right! I don't know where I got that from, I'm not sure what's wrong with my brain these days, I'm going to blame the baby 😂 I've changed them back to event, and directly used the type in the decorator to infer the attributes to use which is much neater. Thanks!

[cs7-shrey]

Hehe 😄
Np