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

Lowercase all case insensitive values in PlatformAPI #903

Open
slarse opened this issue May 3, 2021 · 2 comments
Open

Lowercase all case insensitive values in PlatformAPI #903

slarse opened this issue May 3, 2021 · 2 comments

Comments

@slarse
Copy link
Collaborator

slarse commented May 3, 2021

See #900 for details on usernames.

Anywhere a case insensitive string (such as a username or url) can be passed to the PlatformAPI, we should lowercase it. Anywhere a string is returned (again, such as a username or url), we should also lowercase it.

As #900 showed a bunch of problems with usernames, I'm guessing there may be similar problems with repo names.

@algomaster99
Copy link
Contributor

I tried to approach this problem in a different way. Earlier, I was lowercasing name and URL in any function they appeared. Now, I have written a failing test case and I am thinking of ways to pass it.

Failing test case

    def test_repo_names_are_case_insensitive(self, platform_url, tmp_path):
        funcs.run_repobee(
            f"repos setup -a {const.TEMPLATE_REPOS_ARG.upper()} "
            f"--base-url {platform_url}",
            workdir=tmp_path,
        )

        funcs.run_repobee(
            f"repos clone -a {const.TEMPLATE_REPOS_ARG} "
            f"--base-url {platform_url}",
            workdir=tmp_path,
        )

        def get_repo_name(team, assignment_name):
            return f"{team}-{assignment_name}"

        expected_repo_names = [
            get_repo_name(team, assignment_name)
            for team, assignment_name in itertools.product(
                const.STUDENT_TEAMS, const.TEMPLATE_REPO_NAMES
            )
        ]
        actual_repo_names = [
            student_dir.name for student_dir in funcs.get_repos(platform_url)
        ]
        
        assert sorted(actual_repo_names) == sorted(expected_repo_names)

Right now, it is unable to find a repository TASK-1 so I infer that assignment names need to be lowercased. One way to do so is to explicitly invocate lower for all strings in template_repo_urls. However, I don't think so it's the most generic way to go about it as we would have to do the same thing at many places.

I also tried to alter _APIMeta metaclass but I don't know how to go about it as a metaclass is for changing the behaviour of a class and not an object. In other words, I am not sure how I can interact with arguments inside the metaclass.

@slarse
Copy link
Collaborator Author

slarse commented Jul 26, 2021

You can wrap functions of a class inside the metaclass. A very crude example would be this:

def _lowercase_get_repo_urls(get_repo_urls):
    def _get_repo_urls_lowercase(
        self,
        assignment_names: Iterable[str],
        org_name: Optional[str] = None,
        team_names: Optional[List[str]] = None,
        insert_auth: bool = False,
    ) -> List[str]:
        return [
            url.lower()
            for url in get_repo_urls(
                self,
                assignment_names=assignment_names,
                org_name=org_name,
                team_names=team_names,
                insert_auth=insert_auth,
            )
        ]

    return _get_repo_urls_lowercase

And then you attach it in the metaclass:

        for method_name, method in api_methods.items():
            if method_name in implemented_methods:
                check_parameters(method, implemented_methods[method_name])
                if method_name == "get_repo_urls":
                    attrdict[method_name] = _lowercase_get_repo_urls(
                        implemented_methods[method_name]
                    )

But I think we can make this more generic. Essentially, all strings that are passed into the platform API's functions or are extracted from it with the exception of the token, should be lowercased. We could possibly use type annotations and parameter names to make this generic for all functions of the API.

The one downside with this approach is that it's a lot of magic to add behind the scenes. In general, magic should be avoided when possible because it adds a lot of implicit complexity. But here I don't see a better solution, there are just too many implementations (4 in total at the moment) of the platform API to do this everywhere and test it appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants