Closed Mattadore closed 7 months ago
Ahh that makes sense. While I do agree that "just upping the window" is not a perfect fix either since at any point the window can still be too small, I think adding another setting for something that (I think, given that nobody has mentioned it before) does not happen super frequently is also not the best.
The danger with that is that it increases complexity in configuration for users and also easily leads to issues for people who don't know how to properly set up workspace-specific settings and suddenly have every project of theirs recognized as a gerrit project due to them using global settings instead.
How would you feel about upping the window to (let's say) 50? That may sound like a lot but a simple regex operation that's only performed on initial start probably has a relatively low impact.
I guess my thought was that "if we just default it to false, only people who actually care about it will use it" same as the option that forces the versions. Do you anticipate people setting this option if they don't need it? Can we just make the comment clearer?
I think there's three things I'm afraid of:
.vscode/settings.json
. That can be a bit of an issue for shared repos that use git and that have the settings.json
file checked into the repository. Either everybody now has to use that setting (even if they don't have the extension or want to) or nobody can. With a team of 2 people it's generally fine but (from personal experience) in a bigger company it's not super easy to say "hey please add this thing to the repo for some extension only I want to use".So for those reasons I tend to prefer "magically" fixing things. I wonder what you think though
I think it could be a per-workspace setting rather than per user, with the assumption "hey if you're using git in this workspace you're probably not using multiple different review systems" or they can multi-root. Could also do something like a map to specific workspaces to force enable it.
That said though... 50 is probably enough for any realistic use case, including mine, and I don't know if it's worth over-engineering this. I'm happy to close this if you just want to up it to a larger value.
Alright let's just go for that then, indeed sounds easier to just bump the value than to over-engineer. I'll make the change :)
Some repos can have several non-gerrit commits between gerrit commits (automated commits, merges, etc) so checking only the previous 2 commits for validity can break stuff. We could increase the window... but if we know that all the repos are gerrit repos, we should be able to just skip this validation step.