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

Devel taf #2

Open
wants to merge 19 commits into
base: devel-taf
Choose a base branch
from
Open

Devel taf #2

wants to merge 19 commits into from

Conversation

vini2001
Copy link

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
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
Copy link
Collaborator

diego-garro commented May 2, 2022

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
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
Copy link
Author

vini2001 commented May 4, 2022

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
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.

None yet

3 participants