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

feat: associate with Dockerfile.* #18

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

Conversation

scop
Copy link
Contributor

@scop scop commented Jan 8, 2025

This convention used to be noted in the Docker documentation for some time recently, and has stuck here and there even though the docs no longer refer to it.

docker/docs#9245 (comment)

Closes #6

Warning: untested monkey patch.

This convention used to be noted in the Docker documentation for some
time recently, and has stuck here and there even though the docs no
longer refer to it.

docker/docs#9245 (comment)

Closes dprint#6
@scop scop force-pushed the feat/dockerfile-dot-all branch from 00cbb5e to bf3f1ef Compare January 8, 2025 20:36
@@ -32,7 +32,7 @@ impl SyncPluginHandler<Configuration> for DockerfilePluginHandler {
},
file_matching: FileMatchingInfo {
file_extensions: vec!["dockerfile".to_string()],
file_names: vec!["Dockerfile".to_string()],
file_names: vec!["Dockerfile".to_string(), "Dockerfile.*".to_string()],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is arguably a bit of a hack/abuse (I have a feeling that file_names was not intended to be used with globs in the entries), but looking at dprint main sources, I think it would work. Setting up file_prefixes for something like this would sound a bit overkill to me.

Copy link
Member

@dsherret dsherret Jan 8, 2025

Choose a reason for hiding this comment

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

Yeah, it won't work. This doesn't do pattern matching.

I think the only way to make this work in dprint is for someone to manually add "associations" (https://dprint.dev/config/#associations) under their dockerfile config.

Overall, I'm not too interested in suporting the dockerfile.* convention out of the box because it's a bad convention and it's good docker has switched over to using a file extension.

Copy link
Member

@dsherret dsherret Jan 8, 2025

Choose a reason for hiding this comment

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

For reference, I think this will work today:

{
  "dockerfile": {
    "associations": [
      "**/Dockerfile.*",
      "**/Dockerfile",
      "**/*.dockerfile"
    ]
  },
  "plugins": [
    "https://plugins.dprint.dev/dockerfile-0.3.2.wasm"
  ]
}

Copy link
Contributor Author

@scop scop Jan 8, 2025

Choose a reason for hiding this comment

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

The reason I think it could work is that file_names entries seem to be just concatenated to **/ and it doesn't seem there's anything magical whether the entries are globs or plain filenames: https://github.com/dprint/dprint/blob/14eb074e9baf8d0533f04ad954e17a5a375e6385/crates/dprint/src/paths.rs#L161

Using "associations" works, I'm using that currently, but it's a chore that one has to repeat all the built in globs in it. I find that surprising because it seems they would be appended to the existing builtin globs: https://github.com/dprint/dprint/blob/14eb074e9baf8d0533f04ad954e17a5a375e6385/crates/dprint/src/paths.rs#L163-L164
Alas, if I define

{
  "dockerfile": {
    "associations": [
      "Dockerfile.*"
    ]
    [...]
}

...then only Dockerfile.* match. Need to add Dockerfile and *.dockerfile there to get them back.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yeah, maybe it will work. You can try building by calling:

run: cargo build --target wasm32-unknown-unknown --features wasm --release

Then the wasm file will be somewhere in the target folder (just import the file relatively from your dprint.config file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmph, doesn't seem to work for some reason after all :/

Copy link
Member

Choose a reason for hiding this comment

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

It expands to this glob:

**/{Dockerfile,Dockerfile.*}

I guess that doesn't work. Maybe the dprint cli could be updated to support some basic wildcards in filenames to make it work.

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.

Doesn't work with Dockerfile.*
2 participants