-
Notifications
You must be signed in to change notification settings - Fork 15
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
[fix] Convert case of name and URL of repsoitories to lowercase #934
Conversation
…tandard_repo_names
I'm not quite sure what you mean by that in regards to |
For example, there are methods such as I initially thought I could write a function in the |
Right, now I see what you mean. The purpose of a metaclass is to alter a class definition before it is "solidified". The metaclass' What's necessary there is to add a wrapper function on top of the function defined by the class. This wrapper should lowercase whatever arguments should be lowercased and then pass it on to the actual function. That's non-trivial and could be a PR of its own. |
Okay, we can write a decorator for it. But like you said that can be a different PR, I will just write the system tests for the changes I have introduced and then you can review. |
A decorator isn't the best solution here as it would require the implementations to use the decorator, while we want this to be automatically handled by the metaclass. An option would of course be to have the metaclass check that the correct decorator has been used to annotate the desired functions, but at that point the metaclass can just wrap the function on its own. But, yeah, separate PR. If you want to write system tests, use the Gitea system tests, they cost roughly 10-20 seconds per test. That's much cheaper than the GitLab system tests which consume about 1-2 minutes per tests. The integration tests (still located in |
Sure. But is it assured that if something works well with Gitea, it would also work well with Github?
I was following this PR's format to write tests. Should I shift the tests I have written currently to |
No, of course not. But a) the functionality in this PR is not platform dependent, b) we can't easily system test against GitHub as there's no way to setup a local instance, and c) we need to strike a balance between how expensive the tests are to run, and how important the functionality is.
Yes. The reason I added GitLab system tests for that PR was that it fixed a bug that specifically appeared on GitLab. But each GitLab system test is very expensive, so we only add one when we discover a GitLab-specific bug. |
Just to confirm, do you recommend writing tests in |
I would go with
I realized that there is one case where this will cause strange behavior, and that's when running |
How does one create more repositories in the template organisation? I think I would need to create templates like I am also not sure whether I have to directly clone the repositories in the template organisation to |
You just drop a directory into the Dynamically, something like this should work (but it doesn't due to silly bug, will work soon): def test_setup_with_custom_template_repo(self, platform_dir, platform_url, tmp_path_factory):
# arrange
api = funcs.get_api(platform_url, org_name=const.TEMPLATE_ORG_NAME)
template_repo = api.create_repo("epIcRepo", description="dontcare", private=False)
local_template_dir = tmp_path_factory.mktemp("template")
(local_template_dir / "file.txt").write_text("this is a brand new file!")
branch = "master"
local_template_repo = funcs.initialize_repo(local_template_dir, default_branch=branch)
local_template_repo.git.push(api.insert_auth(template_repo.url), branch)
# act
funcs.run_repobee(f"repos setup -a {template_repo.name} --base-url {platform_url}")
# assert ... Now, as I mentioned, this doesn't work because there's a silly bug in the LocalAPI in that it loads its state from a particular file, and that file doesn't change when the organization changes. Oops. Will fix ASAP.
We only want to verify that RepoBee operates as expected, and there's no way in RepoBee to clone template repositories individually. These integration tests are fully end-to-end using the CLI, we don't fiddle around with internals in them. For now, I'd say you can skip the case where there's a template repo on the platform with weird case. Start with just testing that e.g. Then do that same thing, but with On a side note, you sure picked a deceptively difficult first issue :D |
Right. That's not the job of repobee anyway.
Sounds good. I will give this a try later today.
I know, right? The title of the issue screams "good-first-issue" but that's surely not the case. 😅 |
I will give this a go from scratch and submit a PR soon. I also installed PyCharm today and it helped me understand the flow of the program better. Before today, I was using VSCode 🤦 |
Okay! Let me know if you need any assistance. As is evident, this issue turned out to be pretty complicated :) I've used PyCharm once or twice. It's very nice. |
Which IDE/editor do you use for python projects? |
I use Neovim, or Vim if that's the only thing available. I use that for pretty much everything, I really like having one editor that I know very well and can use for anything. I only use an IDE for Java as it's such a ... heavy language. I'm also not really that into IDEs in general as I find all of the "stuff" to be a little distracting. But that's entirely a matter of taste, I don't think IDEs are bad in any way, and PyCharm is fantastic. Really, all of JetBrains' tools are very good. |
Fixes #903
I am working on two more things which are required for this PR to be merged.
@slarse I tried adding
__post_init__
method inside the metaclass so that I could get iterate over the arguments there and change their case. However, the flow of the program was not calling__post_init__
so I am not sure how to go about it. I could probably override the__init__
to explicitly call__post_init__
but this did not seem like a good way to achieve it.