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

Document naming conventions and repo structure #108

Open
jfly opened this issue Sep 8, 2023 · 16 comments
Open

Document naming conventions and repo structure #108

jfly opened this issue Sep 8, 2023 · 16 comments

Comments

@jfly
Copy link
Member

jfly commented Sep 8, 2023

See discussion on #106 (comment).

We're starting to form some opinions about naming conventions (kebab case, longer, more readable ids). @dmint789 also pointed out that it's really unclear what the "ids" of our icons actually are. For example, we have:

  • event-333: corresponds to WCA event id 333 (documented in the WCIF here, which links to this sql file, which I guess is the ultimate source of truth for WCA event ids)
  • unofficial-333-mirror-blocks: we're the source of truth for these things! But it's unclear if the id people should use in their databases is 333-mirror-blocks or unofficial-333-mirror-blocks.
    • We don't currently enforce uniqueness of names in this repo (in other words, we could create a jfly.svg in both the events and the unofficial directories). We haven't thought about if we'd rather enforce uniqueness across "categories".
@dmint789
Copy link
Contributor

dmint789 commented Sep 8, 2023

Also, we should try to be consistent when it comes to naming events that are extensions of existing events. E.g. 3x3 Blindfolded 2 Person Relay should be 333bf-2-person-relay, since it is simply an extension of the event 3x3 Blindfolded, which has the id 333bf.

@lgarron
Copy link
Member

lgarron commented Sep 12, 2023

Bringing over the discussion from other issues:

I think we should go with underscores for event names. The only legacy compat concern would be mirror blocks, which is unlikely to be used by a significant number of pages yet.

@dmint789
Copy link
Contributor

Why do you want underscores over dashes? The way I see it, it makes no difference

@lgarron
Copy link
Member

lgarron commented Sep 12, 2023

Why do you want underscores over dashes? The way I see it, it makes no difference

Underscores tend to be better for identifiers, since:

  • They can be used in variable and field names in many more common languages than hyphens.
  • Most browsers and operating systems will consider different sets of letter as the same word when connected by underscores (e.g. when double-clicking to select).

For our CSS class names, it would also help isolate the event ID from the icon type prefix.

lgarron added a commit that referenced this issue Sep 12, 2023
Per discussion in #108 as well as several PRs and issues.
lgarron added a commit that referenced this issue Sep 12, 2023
Per discussion in #108 as well as several PRs and issues.
@dmint789
Copy link
Contributor

Oh, I see. Well, I don't have a strong opinion on this as long as we keep things consistent and don't mix naming conventions.

@dmint789
Copy link
Contributor

@jfly Thoughts?

@jfly
Copy link
Member Author

jfly commented Sep 12, 2023

I'm fine with underscores. Let's leave this issue open to track a little more work:

  • let's actually enforce a regex for event ids
  • let's add a blurb somewhere explaining the events vs unofficial folders and how stable we promise the ids to be

@lgarron
Copy link
Member

lgarron commented Sep 12, 2023

  • let's actually enforce a regex for event ids

Sounds like a good idea!

  • let's add a blurb somewhere explaining the events vs unofficial folders and how stable we promise the ids to be

I propose:

  • Select a reasonable name at icon creation time.
  • Once an event is assigned an official ID by the WCA, that's permanent (even if the event is removed). We're happy if this adopts the unofficial ID we used, but if not — we're also glad to have been the testing ground.
  • We'll try to keep renames/removals of unofficial events to a minimum, but no guarantees except:
    • Any removed/renamed unofficial event will remain as an alias for X time (1 year?).

It might be worth keeping track of some of this in code — #112 can generate TypeScript code to refer to icons, and I also want to include a bit of event metadata in the repo (so you can semantically e.g. go from a speed event to its associated BLD event or team event or vice-versa, etc.).

@dmint789
Copy link
Contributor

I feel like we should keep things permanent in general, not just for wca events. This should be the last time we make an icon id change in my opinion, because it's not a good strategy to change something as important as the id of an event, even if it's unofficial.

And what's this point about removals? Why would we ever remove an icon?

@lgarron
Copy link
Member

lgarron commented Sep 12, 2023

I feel like we should keep things permanent in general, not just for wca events. This should be the last time we make an icon id change in my opinion, because it's not a good strategy to change something as important as the id of an event, even if it's unofficial.

I think this is reasonable in principle, but we're almost certainly going to run into edge cases over time. I'm not arguing that we should ever do it lightly, just that we set a baseline for the requirements in case it ever happens.

And what's this point about removals? Why would we ever remove an icon?

The main one I have in mind is removing something from unofficial if it ever becomes official. I still don't know if that's a good idea, but again just setting a baseline.

@dmint789
Copy link
Contributor

Well, then that's not removing an icon, that's just changing which folder it's located in, which I think is totally fine.

And yeah, I agree, it should be possible to rename an icon, but that should be a super rare thing and have very good reasons for doing so. In my case, I use the same id for my events in the db as the icon ids, because I like things to be standardized. But now that we're gonna be renaming all icons with dashes, I'm gonna have to run scripts to update my events, as well as all of the results and rounds for the changed events. So it's not insignificant. And for the same reason I feel like mirror blocks might need to still have a copy using the old kebab-case naming scheme after we make this change.

@dmint789
Copy link
Contributor

Actually, now that I think about it, the people who used mirror blocks aren't gonna be affected until they update to the new npm package version, right? But my point still stands, changes to icon names must be very rare.

@jfly
Copy link
Member Author

jfly commented Sep 12, 2023

I agree that we should strive to not change icon names, but in terms of what we commit to, I like @lgarron's proposals.

I worry we're not all on the same page about icon names vs event ids. They don't feel like the exact same thing to me. For example:

  • Icon name event-333 corresponds to WCA event id 333
  • Icon name unofficial-777bf could correspond to a hypothetical future event id of "777bf"

In other words: this repo defines icon names, and the names lead humans to make a guess about what WCA event ids those might correspond to if the WCA were to ever adopt the event as an official event.

@dmint789
Copy link
Contributor

@jfly wait, is this repo not upstream from the wca? I assumed the wca frontend used this repo for icons. And I thought it would be good for us to set the standards for new event ids.

@jfly
Copy link
Member Author

jfly commented Sep 12, 2023

You are correct that the WCA website uses this project. I think we have a good relationship with the WST and WRT, so I think if any of the unofficial events in this repo ever do become official, there's a good chance they'd be willing to reuse the ids we have here. I just am a little anxious about using strong language like we're "setting the standard for new event ids". That decision is ultimately up to the WST and WRT.

@dmint789
Copy link
Contributor

@jfly yeah, of course I agree that if it did come to that and they used a different id, we'd rename it, but that would also be at the same time as changing the folder from unofficial to event and it would be super rare, so it's okay, I think.

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

No branches or pull requests

3 participants