feat: manual language selection #327

wetneb · 2025-04-16T13:30:49+02:00

wetneb commented

2025-04-16 13:30:49 +02:00

Continuation of #296 by @mathstuf.

wetneb referenced this pull request

2025-04-16 13:32:30 +02:00

manual-language-selection #296

requested review from ada4a

2025-04-17 15:42:35 +02:00

ada4a requested changes

2025-04-17 17:04:44 +02:00

Dismissed

ada4a left a comment

Love the tests, and docs! There were still some things left unaddressed from #296 -- I mostly copied the comments from there verbatim

doc/src/usage.md Outdated

					
				@ -84,0 +84,4 @@

				### Manually specifying the file's language

				You can use the `--language` option (short: `-L`) to specify the language of the files to merge.

				It accepts both file extensions (`--language js`) and language names (`--language javascript`), as specified in the list of [supported languages](./languages.md).

ada4a commented

2025-04-17 16:53:24 +02:00

It accepts [...] file extensions

it doesn't actually though?.. At least right now, LangProfile::find_by_name only looks at name and alternate_names, not extensions

> It accepts [...] file extensions it doesn't actually though?.. At least right now, `LangProfile::find_by_name` only looks at `name` and `alternate_names`, not `extensions`

wetneb commented

2025-04-20 21:35:45 +02:00

Ah, great that you point this out! Wouldn't it make sense to also include extensions then? Arguably, they are language identifiers that users will be familiar with, no?

ada4a commented

2025-04-20 21:57:54 +02:00

Definitely! I mean the only problem with that would be .pl, which is used both for Prolog and Perl. But:

it's recommended to use .pro extension for Prolog because of that
are we seriously going to support Prolog...

Other than that, I think it's pretty much a no-brainer

Definitely! I mean the only problem with that would be `.pl`, which is used both for Prolog and Perl. But: 1. it's [recommended](https://www.swi-prolog.org/pldoc/man?section=fileext) to use `.pro` extension for Prolog because of that 2. are we seriously going to support Prolog... Other than that, I think it's pretty much a no-brainer

src/bin/mergiraf.rs Outdated

					
				@ -53,1 +53,4 @@

				    conflict_marker_size: Option<usize>,

				    /// Override automatic language detection.

				    #[arg(short = 'L', long)]

				    language: Option<String>,

ada4a commented

2025-04-17 17:02:41 +02:00

#296 (comment):

I think the option would make sense for mgf_dev as well (for both subcommands in fact), but we can leave that for another PR

https://codeberg.org/mergiraf/mergiraf/pulls/296#issuecomment-3497861: > I think the option would make sense for `mgf_dev` as well (for both subcommands in fact), but we can leave that for another PR

src/supported_langs.rs

					
				@ -162,6 +163,7 @@ pub static SUPPORTED_LANGUAGES: LazyLock<Vec<LangProfile>> = LazyLock::new(|| {

				        },

				        LangProfile {

				            name: "Java properties",

ada4a commented

2025-04-17 16:59:35 +02:00

#296 (comment):

I'd definitively have a test case for languages whose names contain spaces

https://codeberg.org/mergiraf/mergiraf/pulls/296#issuecomment-3136124: > I'd definitively have a test case for languages whose names contain spaces

ada4a marked this conversation as resolved

src/supported_langs.rs Outdated

					
				@ -349,6 +354,7 @@ pub static SUPPORTED_LANGUAGES: LazyLock<Vec<LangProfile>> = LazyLock::new(|| {

				        },

				        LangProfile {

				            name: "Javascript",

				            alternate_names: &[],

ada4a commented

2025-04-17 17:01:12 +02:00

#296 (comment):

(This is just an example but) I really don't expect anyone to type out "javascript" -- the first instinct will definitively be "js". This raises a question of whether we should allow specifying languages just by their extensions

#296 (comment):

I think the list can certainly be expanded in the future. Probably want a test to make sure there aren't any ambiguous options…

https://codeberg.org/mergiraf/mergiraf/pulls/296#issuecomment-3136799: > (This is just an example but) I really don't expect anyone to type out "javascript" -- the first instinct will definitively be "js". This raises a question of whether we should allow specifying languages just by their extensions https://codeberg.org/mergiraf/mergiraf/pulls/296#issuecomment-3143180: > I think the list can certainly be expanded in the future. Probably want a test to make sure there aren't any ambiguous options…

wetneb commented

2025-04-20 21:36:32 +02:00

Yes, I think that's a good case for including extensions then, no?

ada4a changed title from ~~feat: Manual language selection~~ to feat: manual language selection

2025-04-20 13:36:44 +02:00

wetneb force-pushed manual-language-selection-v2 from 5bc5517e72 to c010642734

2025-04-22 14:52:27 +02:00

Compare

ada4a requested changes

2025-04-22 22:31:45 +02:00

Dismissed

ada4a left a comment

mostly stylistic nits

src/bin/mgf_dev.rs Outdated

					
				@ -215,0 +216,4 @@

				        let repo_dir = tempfile::tempdir().expect("failed to create the temp dir");

				        let test_file = repo_dir.path().join(Path::new("file.txt"));

				        fs::copy(

				            Path::new("examples/java/working/demo/Base.java"),

ada4a commented

2025-04-22 22:21:39 +02:00

you should be able to omit Path::new here and directly above, since the methods take AsRef<Path>, which is implemented for &str

you should be able to omit `Path::new` here and directly above, since the methods take `AsRef<Path>`, which is implemented for `&str`

ada4a marked this conversation as resolved

src/lang_profile.rs Outdated

					
				@ -34,0 +38,4 @@

				    pub fn find_by_name(name: &str) -> Option<&'static Self> {

				        SUPPORTED_LANGUAGES.iter().find(|lang_profile| {

				            lang_profile.name.eq_ignore_ascii_case(name)

				                || lang_profile

ada4a commented

2025-04-22 15:05:28 +02:00

-                || lang_profile
-                    .alternate_names
-                    .iter()
+                || (lang_profile.alternate_names.iter())

```diff - || lang_profile - .alternate_names - .iter() + || (lang_profile.alternate_names.iter()) ```

ada4a marked this conversation as resolved

src/lang_profile.rs Outdated

					
				@ -34,0 +41,4 @@

				                || lang_profile

				                    .alternate_names

				                    .iter()

				                    .chain(lang_profile.extensions.iter())

ada4a commented

2025-04-22 22:22:32 +02:00

You should be able to replace .iter() with & here

You should be able to replace `.iter()` with `&` here

ada4a marked this conversation as resolved

src/lang_profile.rs

					
				@ -237,0 +290,4 @@

				    #[test]

				    fn find_by_filename_or_name() {

				        assert_eq!(

				            LangProfile::find_by_filename_or_name(Path::new("file.json"), None)

ada4a commented

2025-04-22 22:30:43 +02:00

you shouldn't need all the Path::news in this test, since LangProfile::find_by_filename_or_name accepts AsRef<Path>, so a &str works on its own

you shouldn't need all the `Path::new`s in this test, since `LangProfile::find_by_filename_or_name` accepts `AsRef<Path>`, so a `&str` works on its own

ada4a marked this conversation as resolved

src/lang_profile.rs Outdated

					
				@ -237,0 +301,4 @@

				                .name,

				            "JSON"

				        );

				        LangProfile::find_by_filename_or_name(

ada4a commented

2025-04-22 22:16:17 +02:00

expect_err is kind of confusing -- I think using assert!(/* ... */.is_err(), /* the message */) might be clearer

`expect_err` is kind of confusing -- I think using `assert!(/* ... */.is_err(), /* the message */)` might be clearer

ada4a marked this conversation as resolved

src/merge.rs

					
				@ -41,3 +38,1 @@

				            fname_base.display()

				        );

				        return line_based_merge(contents_base, contents_left, contents_right, &settings);

				    let lang_profile = match LangProfile::find_by_filename_or_name(fname_base, language) {

ada4a commented

2025-04-22 22:17:43 +02:00

couldn't you use let Ok(lang_profile) = /* ... */ else { /* ... */ }?

couldn't you use `let Ok(lang_profile) = /* ... */ else { /* ... */ }`?

wetneb commented

2025-04-23 08:04:54 +02:00

I thought about it and the problem is that I need the error message in the else block… do you have a solution for that?

I thought about it and the problem is that I need the error message in the `else` block… do you have a solution for that?

ada4a commented

2025-04-23 08:39:37 +02:00

Oh, completely missed that! It looks like that match is indeed required then, yeah.

By the way, message already ends with a ., so you'll end up with two of them – I think it makes more sense to remove the ones in find_by_filename_or_name

Oh, completely missed that! It looks like that `match` is indeed required then, yeah. By the way, `message` already ends with a `.`, so you'll end up with two of them – I think it makes more sense to remove the ones in `find_by_filename_or_name`

wetneb commented

2025-04-27 09:34:06 +02:00

Good eyes! :)

❤️ 1

ada4a marked this conversation as resolved

src/solve.rs Outdated

					
				@ -28,3 +26,1 @@

				            fname_base.display()

				        )

				    })?;

				    let lang_profile = if let Some(lang_name) = language {

ada4a commented

2025-04-22 22:26:32 +02:00

can't this whole block be replaced with LangProfile::detect_from_filename_or_name?

can't this whole block be replaced with `LangProfile::detect_from_filename_or_name`?

wetneb commented

2025-04-27 09:33:51 +02:00

Very good catch, of course that's much better.

❤️ 1

ada4a marked this conversation as resolved

wetneb added 3 commits

2025-04-27 09:33:09 +02:00

Stylistic improvements 34b9ab1bf9

Remove duplicate dot 23604f7f9a

Also use find_by_filename_or_name in solve.rs

/ test (pull_request) Successful in 58s

Details

538e243927

ada4a requested changes

2025-04-27 15:50:21 +02:00

Dismissed

ada4a left a comment

more of the same

src/bin/mgf_dev.rs Outdated

					
				@ -215,0 +223,4 @@

				                "parse",

				                "--language",

				                "java",

				                test_file.display().to_string().as_str(),

ada4a commented

2025-04-27 15:46:56 +02:00

I'm pretty sure .display() is plain redundant here, since to_string is implemented in terms of it -- so you can just remove that.

You could also replace to_string().as_str() with to_str().unwrap() (first one replaces non-Unicode characters with the ? thing, while the second one returns None on non-UTF8 characters, both of which won't happen in this case), but it's not like one is really shorter than the other... Replacing .as_str() with &, on the other hand, will save some characters

I'm pretty sure `.display()` is plain redundant here, since `to_string` is implemented in terms of it -- so you can just remove that. You could also replace `to_string().as_str()` with `to_str().unwrap()` (first one replaces non-Unicode characters with the `?` thing, while the second one returns `None` on non-UTF8 characters, both of which won't happen in this case), but it's not like one is really shorter than the other... Replacing `.as_str()` with `&`, on the other hand, _will_ save some characters

ada4a commented

2025-04-27 15:51:54 +02:00

I see that you used to_str().unwrap() in mergiraf.rs, so maybe go with that for consistency... up to you

I see that you used `to_str().unwrap()` in `mergiraf.rs`, so maybe go with that for consistency... up to you

ada4a marked this conversation as resolved

src/lang_profile.rs Outdated

					
				@ -237,0 +301,4 @@

				        );

				        assert!(

				            LangProfile::find_by_filename_or_name(

				                Path::new("file.json"),

ada4a commented

2025-04-27 15:38:54 +02:00

missed one Path::new here:)

missed one `Path::new` here:)

ada4a marked this conversation as resolved

src/lang_profile.rs Outdated

					
				@ -237,0 +307,4 @@

				            .is_err(),

				            "If a language name is provided, the file name should be ignored"

				        );

				        LangProfile::find_by_filename_or_name(Path::new("file.unknown_extension"), None)

ada4a commented

2025-04-27 15:39:52 +02:00

this could also be turned into an assert!, and Path::new can be removed as well

this could also be turned into an `assert!`, and `Path::new` can be removed as well

ada4a marked this conversation as resolved

wetneb added 1 commit

2025-04-28 17:40:23 +02:00

Style fixes

/ test (pull_request) Successful in 1m4s

Details

d7e2290b5e

ada4a reviewed

2025-04-28 19:28:06 +02:00

src/lang_profile.rs

					
				@ -237,0 +276,4 @@

				        assert_eq!(LangProfile::find_by_name("python").unwrap().name, "Python");

				        assert_eq!(LangProfile::find_by_name("py").unwrap().name, "Python");

				        assert_eq!(

				            LangProfile::find_by_name("Java properties").unwrap().name,

ada4a commented

2025-04-28 19:27:59 +02:00

When I wrote about the need for tests for languages whose names contain spaces, I was thinking more about cli tests... but I mean the users will probably be able to realize that such names need quoting. So yeah, I guess that doesn't really need tests after all, and having this test is also good

ada4a requested changes

2025-04-28 19:30:29 +02:00

Dismissed

ada4a left a comment

One final change, and we can finally merge this. Thank you for bringing this to completion:)

src/supported_langs.rs Outdated

					
				@ -542,6 +559,7 @@ pub static SUPPORTED_LANGUAGES: LazyLock<Vec<LangProfile>> = LazyLock::new(|| {

				        },

				        LangProfile {

				            name: "Typescript (TSX)",

				            alternate_names: &["TSX"],