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 missing Modula-2 *.def extension #6671

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dkearns
Copy link

@dkearns dkearns commented Jan 4, 2024

Modula-2 splits the definition of a module into separate interface and implementation files having extensions *.def and *.mod respectively.

Linguist currently only detects *.mod files. This PR adds support for *.def file detection.

See #3657 and #6451 for related discussions.

I'm not sure why END is in those patterns. Assuming a top down search the MODULE line will obviously be found first but I may be missing something about the implementation.

Description

Checklist:

@dkearns dkearns requested a review from a team as a code owner January 4, 2024 18:28
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See inline comment

- extensions: ['.def']
rules:
- language: Modula-2
pattern: '^\s*(?i:DEFINITION\s+MODULE|END)\s+[\w\.]+\s*;'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This heuristic will never be used as this is the only user of this extension within linguist which in itself is potentially a problem as .def is an incredibly generic extension used by many things.

If you're 100% certain this regex will only match Modula-2 files, then adding this extension to the generic.yml file will ensure this regex takes effect and is used. If there's a risk this misidentifies other languages, it's best to leave off this extension and use an override when needed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for missing that, I thought I'd tested a failing case but obviously not.

The pattern for *.def is more specific than that applied to *.mod filenames and there are only 30% the number of files on GitHub. I tested as well as I can with GitHub search and didn't find any false positives.

The current matching for *.mod is also very poor. Most of the GNU Modula-2 repository appears to match as AMPL, for example. So, I've also fixed that pattern to limit it to Modula-2 syntax.

Some breaking changes were made to address #3433 but they don't make any sense to me. The issue reporter seems to be using Active Oberon, a different language. They may be asking for these files to match as Modula-2 because the syntax is similar, Oberon being the next in the line of Niklaus Wirth's languages but not recognised by Linguist.

However, Oberon is is no more Modula-2 than it is Pascal. Modula-2 does not support a . in the module name and I can find no examples of any old non-standard Modula-2 dialects supporting that either.

@dkearns dkearns force-pushed the add-missing-modula2-def-extension branch from f607d34 to a03e63e Compare January 10, 2024 15:27
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a heuristic test for this extension.

Improve *.mod pattern to match implementation modules.  These were only
being incidentally matched via the /END\./ pattern branch.

See github-linguist#3657 and github-linguist#6451 for related discussions.
@dkearns dkearns force-pushed the add-missing-modula2-def-extension branch from a03e63e to 313cbf8 Compare January 10, 2024 15:37
@dkearns
Copy link
Author

dkearns commented Jan 10, 2024

Please add a heuristic test for this extension.

Done, thanks.

@lildude
Copy link
Member

lildude commented Jan 10, 2024

Sorry. I should have been clearer... we need the heuristic test and fixtures as implemented by other generic extensions. See what has been done for the likes of .sol and .cmp.

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

2 participants