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

Group GLCT definitions together #1074

Closed
wants to merge 4 commits into from
Closed

Group GLCT definitions together #1074

wants to merge 4 commits into from

Conversation

ZeLonewolf
Copy link
Member

This is a minor refactor to put all the GLCT (Great Lakes Circle Tour) shield definition code together in the same place in shield_defs.json. They were thousands of lines apart which made it confusing to understand what was going on.

image

@claysmalley
Copy link
Member

All of the overrideByRef, overrideByName and refsByName cases are at the end of the file. Either we should keep all of them together, or get rid of this section and put all of the special cases inline.

@ZeLonewolf
Copy link
Member Author

Ah, I see. It wasn't obvious to me that's what was going on here. At the least, we ought to group all parts of a shield definition together. For example, in the Nova Scotia section we find:

  shields["CA:NS:S"] = {
    notext: true,
  };

Totally unclear that there's additional definition at the end of the file. I think the original notion was that we wanted to tightly limit the number of special cases and that's why we segregated them. 🤔

@ZeLonewolf ZeLonewolf marked this pull request as draft May 20, 2024 02:28
@quincylvania
Copy link
Contributor

I could see the number of "special cases" growing to the point where they're not so special. I think it makes more sense just to organize everything consistently.

@ZeLonewolf
Copy link
Member Author

Superceded by #1081

@ZeLonewolf ZeLonewolf closed this May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants