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

Fixes #5557 - Rename standing agreement files to use full sector name #5558

Merged
merged 107 commits into from
Jan 22, 2025

Conversation

aidenbok203
Copy link
Collaborator

Fixes #5557

@aidenbok203 aidenbok203 added the enhancement For new or improved features label Jan 7, 2025
@hazzas-99
Copy link
Contributor

Changed Stafa to TCSTA but can change back

@aidenbok203 yeah please do, "Stafa" and "Trent" are the names of the two sectors that make up our PC Southeast (MAN_SE_CTR)

@aidenbok203
Copy link
Collaborator Author

ha, will do, not quite sure what I was thinking when I did that

Copy link
Collaborator

@BenWalker01 BenWalker01 Jan 7, 2025

Choose a reason for hiding this comment

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

Should this be COWLY?

Copy link
Contributor

Choose a reason for hiding this comment

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

TCCOW is good, as this matches TCWEL/TCRED/etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But isn't the point of this PR to make thing consistent, I don't see why we wouldn't do this fully consistently?

Copy link
Collaborator Author

@aidenbok203 aidenbok203 Jan 8, 2025

Choose a reason for hiding this comment

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

TC COWLY is a TC, while Stafa and Trent aren’t part of TC?

Copy link
Contributor

Choose a reason for hiding this comment

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

COWLY is TC, but STAFA and TNT are PC sectors; for consistency, wouldn't either TCCOW/PCSTA or COWLY/STAFA instead of a mixture of both be preferable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

wouldn't either TCCOW/PCSTA or COWLY/STAFA

Yeah, that was my thought. I though we were renaming to full sector names, hence COWLY.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or could even be TCCOWLY PCSTAFA to be completely obvious.

Copy link
Contributor

@hazzas-99 hazzas-99 Jan 9, 2025

Choose a reason for hiding this comment

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

I don't really want to follow that logic to PCHumber or PCAntrim though - all of the sector names at Prestwick are proper names of places rather than just fixes/VORs, with the exception of 'South East', which is split into STAFA and TRENT sectors. I don't think we need a 'PC' qualifier before Prestwick sectors because none of the fully split names are duplicated at Swanwick.

For London AC sectors, we just have the sector number, because putting 'S' before it would be superfluous. I don't see a need to change this.

Then somehow at some point, we ended up with TC before the sector name for London TC, probably because when we didn't have the sectors fully split out, just NE for TC Northeast would've looked weird and is confusingly similar to our PC Northeast - and so the convention we've ended up with is having TCNE, TCRED, TCLAM, etc.

For that reason, until such time that we fully split out our TC sectors into their constituent parts, I suggest we continue to use the TC prefix - because NE or SW on its own is confusing! Eventually we could move to NEDEPS, SWDEPS, OCK, WILLO, TIMBA, but we're not there yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems sensible, and I wouldn't oppose maintaining that status quo, though I don't think keeping "TCNE" and the like is necessarily incompatible with expanding the individual TC sector names.

Also, whilst we're tidying up the file names, might it be an idea to make consistent "Solent"/"SOLENT" and "13+14"/"E (13+14)"?

Agreements/Internal/Deancross_WC-Tay.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@hazzas-99 hazzas-99 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@PLM1995 in case you merge before the other agreement PRs, I'm happy to go and fix the file naming in those after

@PLM1995
Copy link
Collaborator

PLM1995 commented Jan 8, 2025

@PLM1995 in case you merge before the other agreement PRs, I'm happy to go and fix the file naming in those after

I'll do those others first once they're tested & approved.

@PLM1995
Copy link
Collaborator

PLM1995 commented Jan 9, 2025

This PR won't re-order any agreements and break anything will it (@hazzas-99)? I think the alphabetical ordering of file names is important as ES does the first matching agreement it finds, and the compiler makes the .ese from the files in alphabetical order within folder order specified in the compiler config.

@hazzas-99
Copy link
Contributor

This PR won't re-order any agreements and break anything will it (@hazzas-99)? I think the alphabetical ordering of file names is important as ES does the first matching agreement it finds, and the compiler makes the .ese from the files in alphabetical order within folder order specified in the compiler config.

@PLM1995 none of the files are excluded and then re-included in the config file and those are generally the ones that require specific ordering. Since we're just expanding abbreviations to their full form I should think the files will remain in almost exactly the same order otherwise!

"misc": {
"agreements": [
{
"type": "folder",
"folder": "Agreements/Airport",
"recursive": true
},
{
"type": "folder",
"folder": "Agreements/Internal",
"recursive": true,
"exclude": [
"TCCOW_TCCPT.txt"
]
},
{
"type": "files",
"files": [
"Agreements/Internal/TCCOW_TCCPT.txt"
]
},
{
"type": "folder",
"folder": "Agreements/External",
"recursive": true,
"exclude": [
"1_RR-Q.txt",
"4_SNE.txt",
"6_RR-V.txt",
"7_SNE.txt",
"8_SNB.txt",
"12_AA.txt"
]
},
{
"type": "files",
"files": [
"Agreements/External/1_RR-Q.txt",
"Agreements/External/4_SNE.txt",
"Agreements/External/6_RR-V.txt",
"Agreements/External/7_SNE.txt",
"Agreements/External/8_SNB.txt",
"Agreements/External/12_AA.txt"
]
}
],

@kristiankunc
Copy link
Collaborator

@aidenbok203 you know you can stage multiple files for a single commit 😂

.github/CHANGELOG.md Outdated Show resolved Hide resolved
@PLM1995 PLM1995 merged commit 0be1cb3 into VATSIM-UK:main Jan 22, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement For new or improved features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

admin: Rename standing agreement files to use full sector name
6 participants