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

Add airways parsing (+ a few other tweaks) #16

Merged
merged 3 commits into from
Apr 18, 2024

Conversation

philsjh
Copy link
Contributor

@philsjh philsjh commented Apr 8, 2024

First contribution!

  • Add airway parsing to the SCT parser.
  • Expand the matching in the pest parser to accommodate this
  • Expand the parser to allow inline sct comments
  • Alter parsing flow to parse independent parts of the sector file first, then re-parse for anything that depends on other parts of the sector file (for example: Airways can have fixes, VORs, or NDBs, but we don't know if the fixes are legit if we can't check).
  • Update test cases to support new functionality

I'm not as familiar with rust as I am with other languages, so comments/opinions are welcome!

@globin
Copy link
Contributor

globin commented Apr 15, 2024

Sorry for taking so long to respond to this, I had to think a while which approach I'd like to take with this. I believe this pull request inadvertently brings up a larger decision, namely if we should validate and ensure consistency while parsing.

I have ignored consistency so far as up to now these checks would have needed sct data in addition to parsing ES topsky maps and additionally a lot of files I have tested actually are inconsistent.

For the sector file parsing I'd prefer keeping this approach as this also reduces complexity by quite a bit. I've pushed what I'd propose to https://github.com/blip-radar/vatsim-parser/compare/sct-airways. This uses the Location enum (Fix or Coordinate) which I had previously already used for ES topsky maps.

All that being said, I do believe that checking consistency is a valid concern and should be implemented here, but decoupled from the parsing logic. Issue to keep track of this in #18.

Are there any issues I have missed in my rambling?

@philsjh
Copy link
Contributor Author

philsjh commented Apr 18, 2024

Hey Robin,

No problem, yeah I think that all makes a lot of sense, and using a location enum is probably best.

@caspianmerlin might have a further opinion on this.

Cheers,
Phil

@globin globin mentioned this pull request Apr 18, 2024
@globin globin merged commit d053759 into blip-radar:main Apr 18, 2024
3 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