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

chore: Merges main-overview module with control-planes module #1743

Merged
merged 2 commits into from
Nov 9, 2023

Conversation

johncowen
Copy link
Contributor

@johncowen johncowen commented Nov 7, 2023

We've been chatting for a while about moving main-overview to be called control-planes, and now felt like the first good opportunity to do it.

This PR merges main-overview module with control-planes module, so a main-overview module no longer exists.

Notes:

  • The control-planes module is already being added in main.ts
  • I moved the MainOverview component (now ControlPlaneStatus) to no longer be injected via the @/components/index file. Eventually this file will cease to exist.
  • I didn't rename the i18n keys prefixed with main-overview as yet, I'll do that in a follow up.

There were a couple of "while I was here" things, that I'll note inline.

@johncowen johncowen requested a review from a team as a code owner November 7, 2023 14:23
@johncowen johncowen requested review from kleinfreund and removed request for a team November 7, 2023 14:23
Copy link

netlify bot commented Nov 7, 2023

Deploy Preview for kuma-gui ready!

Name Link
🔨 Latest commit e92c6e7
🔍 Latest deploy log https://app.netlify.com/sites/kuma-gui/deploys/654cc9a1e83e270008b35a6a
😎 Deploy Preview https://deploy-preview-1743--kuma-gui.netlify.app/gui
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor Author

@johncowen johncowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlines:

src/app/control-planes/sources.ts Show resolved Hide resolved
src/app/control-planes/views/item/IndexView.vue Outdated Show resolved Hide resolved
@johncowen johncowen force-pushed the chore/rename-main-overview branch from c025910 to e0ef00e Compare November 7, 2023 14:27
@johncowen johncowen changed the title chore: Merges main-overview module with control-planes modules chore: Merges main-overview module with control-planes module Nov 7, 2023
@johncowen johncowen marked this pull request as draft November 7, 2023 14:31
@johncowen johncowen force-pushed the chore/rename-main-overview branch 2 times, most recently from 264b1af to a691d9b Compare November 7, 2023 14:42
Signed-off-by: John Cowen <[email protected]>
@johncowen johncowen force-pushed the chore/rename-main-overview branch from a691d9b to a434c86 Compare November 7, 2023 15:02
@johncowen johncowen marked this pull request as ready for review November 7, 2023 15:15
Copy link
Contributor

@kleinfreund kleinfreund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This mostly looks good to me.

There is one thing that I want to address here: I don’t want to keep naming files IndexView.vue. I mentioned multiple times how this impacted my ability to find where stuff is in the code base significantly.

Please stick to the pretty well-established pattern to include the module name in one form or another (e.g. ControlPlaneDetailView.vue).

@johncowen
Copy link
Contributor Author

Just to link to a previous conversation in a feature PR where this came up, it also came up offline previous to that (which is also mentioned in that PR)

#1682 (review)

Just to note this is a refactoring PR, specifically about renaming things to our newer style, although granted not specifically about the names of .vue files.

#1682 decided not to follow the naming convention of the zones, zone-insights and zone-egresses modules. That being - not repeating the namespace of the file in the name of the file. I didn't see a technical reason why there was a problem with not repeating the namespace of the file in the name of the file and I don't think you've given on as yet, but I decided to approve the PR in order to get the feature into the software and then circle back.


Here is a screengrab of my editor now:

Screenshot 2023-11-08 at 16 40 29

Note from #1682 I've no idea what the extension is on the ZoneIngressSummar name when looking at that. I don't really know whether the name ends in Summary. Is this a .spec file? Is it a ZoneIngressSummaryAbstractView, I've no idea. I've also noticed that this kinda problem is also beginning to happen in the GH web UI in the file explorer during code reviews.

Here's a screengrab of the the zones module in my text editor previous to me removing the namespace from the name of the file for the zones related modules.

Screenshot 2023-11-08 at 15 08 07

I still find it really hard to look at that screengrab, so I'm really glad I changed it. Both of the above screengrabs show how redundant it is to prefix every file within a folder with the same name of the folder.

Could I widen the sidebar in my editor? Of course I can, but its an extra thing to keep having to do, but I could. Pretty sure I can't on GH though. Would widening my editor sidebar it make that last screengrab any easier to parse quickly, I'd say no.


On the other hand, in our past conversations you've pointed out that you have found it hard to find the files in your fuzzy finder with these shortened file names, although I did point out that as the fuzzy finder will also use the name of the folder in its search, there should be no real difference and I didn't get any response back from you on that, so I kinda assumed that was problem solved.

Does ZoneListView technically contain more characters than IndexView? Yes. Does that make ZoneListView a worse name? No. Doesn’t make it any harder for you to find it and does make it easier for me to find it.

The technical fact there is that is that IndexView has less characters than ZoneListView and therefore is easier to read in a sidebar. The personal opinions are that ZoneListView isn't a 'worse name' or that it makes it harder or easier for someone to find. And those points are all omitting that fact that its redundant to include the namespace of a thing in the name of the thing that has already been namespaced. Or that with IndexView as opposed to TabView (the UI might change), or AbstractView (abstract generally means its not directly used) is maybe a safer more descriptive choice.


Please stick to the pretty well-established pattern to include the module name in one form or another (e.g. ControlPlaneDetailView.vue).

I wouldn't call ControlPlaneDetailView exactly well established, I could probably point to a few repos where they don't repeat the namespace in the name of the file (e.g. https://github.com/Kong/kong-manager/tree/main/src/pages/certificates). In this repository specifically I'd say the split is probably 60/40 amongst our modules, Its mostly only 60/40 due to the fact that we've spent quite a long time organising this repository a little better that it was previously and we are now nearing the end of that, and naming things a little better was one of the last things on my list.


After all that being said I'd be fine to change this to use ControlPlaneDetailView for the moment if you still have an issue with any of the above, but if so, I think another MADR would then be a good idea instead of flip-flopping backwards and forwards with this. This would give another opportunity to provide some technical reasoning around this rather than just "I prefer it"

Let me know either way and we can either move on here, or I can change the name, move this to a MADR around the value of namespaces and then either change it back or leave as is depending on the outcome of that.

@kleinfreund
Copy link
Contributor

Your arguments for why you prefer the index-style file names seem to be:

    1. preference to defer to the containing directory as the sole name-giving element
    1. editor configuration not allowing you to see moderately long file names
    1. short file names being more readable than long ones

Point 1 is something that’s not impacted by different file names so it doesn’t matter if the file is called IndexView or ZoneListView

Point 2 is something that you control. You can configure your editor to show file names longer than X characters adequately. In the GitHub UI for diffs, there are always multiple places to see the file name, so should it be cut-off in some cases, it won’t prevent you from identifying what the file is about.

Point 3 is only really true in the most literal sense of being able to read and parse fewer characters more quickly. But really, a name IndexView that appears perhaps dozens of times in a code base is so much less clear than a name ZoneListView that appears once. Plus ZoneListView has the advantage that you don’t need to look at anything else to know what this file is.

Also, I very much understand that it’s possible to identify an IndexView file in purpose by looking at its containing directory and path. My point is that I shouldn’t need to look at both. Identifying files by name is easy and I don’t want to change this habit unnecessarily. File names are commonly emphasized in UIs like project-wide full-text search and command palettes. File names should be the primary identifier of the file. That also makes it much easier to move it around because moving it around shouldn’t change the meaning of its contents.

I’m going to write an MADR for this.

For now, there is no action needed here.

@johncowen
Copy link
Contributor Author

I’m going to write an MADR for this.

Sounds good!

Do you want me to change the name so we can merge in the meantime, or shall we wait for the MADR?

I'd prefer to change the name to here ControlPlaneDetailView at least temporarily (or maybe not) so I can merge this. I've been waiting for a good time to do this PR and its been kinda hard to find a time when its not gonna clash with other PRs, and i would want it to get in the way of anything else.

Let me know, Im good either way. Separately, ping me when that MADR is there 👍

@kleinfreund
Copy link
Contributor

We should generally not wait for an MADR in already on-going work so we can take our time with it.

@johncowen
Copy link
Contributor Author

Ok lemme change the name here so we can merge. Just in case apart from that it sounded like the rest was good to go? Shout me if there was anything else here, otherwise I'll ping you when I've changed up the name.

@johncowen johncowen merged commit 4a68417 into kumahq:master Nov 9, 2023
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants