-
Notifications
You must be signed in to change notification settings - Fork 3
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 #132 - Implement ArcGen #210
Conversation
Full circles with radius and centre: `;Arc centre N051.29.02.000 W000.36.14.000 radius 1.25`
Not sure about the testing in SectorDataFileTest, feel like it could be done better, but the rest isn't too bad :) |
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.
LGTM so far - just some thoughts! :)
/* | ||
* Returns whether or not the line is an arc gen line | ||
*/ | ||
public bool IsArcGenLine(string line) { |
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'm wondering if there's an alternate way to express arcs that don't require them to be comments 🤔 I think if we start to make some comments "special", things could get confusing.
I think we could look at creating a macro expression (I hate macros, but they're great for this), that gets expanded. That way, we don't have this situation where some comments are special.
Perhaps something like @ARC(arc def here)
?
For a first pass, we can just parse macros in the parser, but perhaps in the future we can implement something that expands all macros on a line, before we send things over to the parser?
LMK what you think :)
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, I was also thinking the syntax of the arcs could be changed, I was just going off of what went into the ArcGen tool. Eventually yeah I could also write a macro expander - I had a look in the rest of the replacers but they were only for options in the file.
🎉 This PR is included in version 1.16.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Fixes #132
Implement the ArcGen algorithm with some improvements. Features: