-
Notifications
You must be signed in to change notification settings - Fork 123
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 #4822 - Update all incorrect fixes for airport RNAV sids / RNAV approaches #4823
Conversation
AD2 fix parser has been added, it currently does not use the AIRAC class as adding it is going to start getting messy with all the different PRs. After this commit is pushed as well as #4817, I'll update all the tooling on a separate PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes all accurate.
_data/Tools/AD2fixparser.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you're going to want to refactor this once the other tooling stuff is included? If so lets put this PR 'on hold' or 'draft' until ready :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to update the tooling in a different PR, as here I don't have access to the other classes. Either removing the AD2 parser from here or just pushing like it is now and then updating it later would work, but not pushing this one would leave me in a trap of not being able to get the backend code and would delay way longer :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK so you want me to merge this now? With or without the parser? Whatever's easier for you.
I thought it was #4817 you needed first, then update this using that stuff, but maybe I'm getting confused!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep you're right with that, I'll just explain fully here:
What I want to do is create a tooling system that uses a framework for parsing the eAIP (an API). This framework would have backend functions and other files and code that would be general across all parsers, not specific. If I were to update the tooling in each PR individually, I would have to duplicate these backend functions across every PR, which would a. cause lots of conflicts and b. if there were any changes requested of the backend functions, I would have to make those changes across all the different PRs which would take ages and be super inefficient.
So, to summarise, I don't mind whether you keep the parser here or not, its getting replaced soon anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, OK, so what I had in mind was wait until those backend things are in place and introduce the new tooling with these PRs that use tooling. We have over 2.5 weeks to the next release, so we're not particularly in a rush to merge the actual changes to our Fixes.txt files. I was thinking get it right first time / in one PR rather than rushing to merge it peicemeal.
Of course if you want to get them merged now that's fine, and do all the tooling separately, if that's easier for you. In that case I personally wouldn't bother including the parser that's soon to be removed/replaced/refactored, unless you need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I've taken out the parser (I have a local copy), I'll update it all in a different PR, just have to wait for either #4817 to be merged or I'll also remove the parser from there and just stick it all on one now.
Fixes #4822
Summary of changes
Screenshots (if necessary)