Skip to content

Devel taf#2

Open
vini2001 wants to merge 19 commits into
TAF-Verification:devel-taffrom
vini2001:devel-taf
Open

Devel taf#2
vini2001 wants to merge 19 commits into
TAF-Verification:devel-taffrom
vini2001:devel-taf

Conversation

@vini2001

Copy link
Copy Markdown

Set of changes to complete the TAF parser
(small bits may still be missing, but a lot of edge cases were handled)

@diego-garro

Copy link
Copy Markdown
Collaborator

Hi @vini2001 and @lawrenceCA. Thanks for the PR, I'm going to check the commits. Sorry I took some days out of code, so until today I saw the PR. Greetings!

@diego-garro

diego-garro commented May 2, 2022

Copy link
Copy Markdown
Collaborator

The Idea for this package is make a TAF-Verificator command line application, but an API package too to use in Dart projects. And as you can see, there is the same project in Python language. And I hope to create the same in TypeScript and Go. Your help is always welcoming 👍🏼

@diego-garro

Copy link
Copy Markdown
Collaborator

I have finished to review the commits. Sadly I can't accept your Pull Request, and it is for several reasons.
First, it has a lot of changes, and a lot of commits, some changes may drive to future bugs because they are not according with the rest of the code.

I see one change in the VALID regex of the TAF, the from time cannot be 1224 for example, must be 1300, and you added a 4 in the regex letting parse the incorrect code. See item 51.8, rule 51.8.1, Manual on codes Volume I.1.

As a recomendation, first open an issue with a specific change to make. Next, do your changes in a few commits, say about 3 commits, no more and send your Pull Request.

I apologize too, because I have not added a CONTRIBUTE.md file where I put the rules of how to contribute to the project. I will be doing that in the next few days.

One more time, thanks for your PR, your help will be welcoming always. I hope not to discourage you to continue contributing.

@vini2001

vini2001 commented May 4, 2022

Copy link
Copy Markdown
Author

Hi @diego-garro .
No worries, we needed the project complete and so we decided to finish the work to parse all the possible TAF codes that could appear. I created the PR more as a way to contribute to the project since it really helped us.

However, I suggest you take a look at my fork for your following work as it will save you some time :)
Regarding the from time being 1224, I can't tell about the actual rules, but there are several examples on the exhaustive test file I created where this is used, so without allowing the 4, the project won't parse all possible TAF codes.

Hope this helps, we'll continue to maintain our fork as we spot issues.
All the best, Vinícius.

@diego-garro

Copy link
Copy Markdown
Collaborator

Excellent @vini2001, some features and fixes are very helpful. Greetings.

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.

3 participants