-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 .gml in Gerber Image #4462
Add .gml in Gerber Image #4462
Conversation
5,1,8,0,0,1.08239X$1,22.5* | ||
% | ||
%ADD10C,0.00000*% | ||
D10* |
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.
Normally, we prefer larger samples over smaller ones, but a 1,000+ line file is overkill (and adds unnecessary bloat to the classifier). Any chance you could cull some of the X0213131...
lines so the file is cut down to maybe 100-200 lines, maybe?
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.
Done :)
@@ -166,6 +166,8 @@ disambiguations: | |||
pattern: '(?i:^\s*(\<\?xml|xmlns))' | |||
- language: Graph Modeling Language | |||
pattern: '(?i:^\s*(graph|node)\s+\[$)' | |||
- language: Gerber Image | |||
pattern: '\*\%$' |
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.
So, the only exception in Game Maker Language would be that if you had a string that ends with *%
somewhere?
E.g.;
//my code
my_function('abc'); //weird chars *%
This would then be detected as Gerber Image?
I think its quite the edge case though, can't think of this pattern occuring often in GML files.
Is there a way to check what this change would do to existing GML files? Don't think it will, but still :+)
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.
The only way the *%
would be valid in GameMakerLanguage, would be in a string (which is always single-quoted in GML) or in a comment (which is //
or /* */
format).
Maybe if /*
or //
occurs, we can safely say its Game Maker Language? Not sure if that would be occuring in the Gerber Image format, but if its not, it might be a nice exception to add.
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'd say, use an actual Gerber Image command, like:
G04 COMMENT*
X4Y2405D02*
X1264Y2405D01*
Patterns to match them, taken proudly from my hand-written Gerber highlighting grammar:
^G04.*?\*$
^[XY][-+]?[0-9]+\*$
Those patterns need to have their "m" switch enabled so ^ and $ match against the start and end of each line, rather than the entire file. More documentation on the Gerber Image format can be found here.
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 think its quite the edge case though, can't think of this pattern occuring often in GML files.
I know nothing about GML but it certainly does sound like a small enough edge case for us to not worry about right now.
I'm happy to merge this if others are.
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.
Sorry for the late reply, but as a Game Maker Language comment block actually can start with a *, a % might not be that uncommon;
/**
*function description
*% of data returned
*/
If /*
, */
or //
is never used in the Gerber format, then I'd say:
if "*%" matched AND NOT ("/*" OR "*/" OR "//") then language = Gerber
That might exclude any false positives.
If the Gerber format could contain characters like that it might be better to look at a different solution
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.
@RobQuistNL I'd argue that that's still too much of an edge-case to warrant complicating our heuristics that much. Particularly since this is more readable:
/**
* function description
* % of data returned
*/
In any case, the patterns I gave above won't match the sort of comments you've mentioned.
This pull request has been automatically marked as stale because it has not had recent activity, and will be closed if no further activity occurs. If this pull request was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions. |
Description
Fix #4458.
Checklist: