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.

Sign up for GitHub

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
Open

feat: associate with Dockerfile.* #18

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/wasm_plugin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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:

dprint-plugin-dockerfile/.github/workflows/ci.yml

Line 37 in d208687

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.

},
}
}
Expand Down
Loading