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

license should be from relative dir #4967

Closed
wants to merge 2 commits into from

Conversation

gilescope
Copy link

As mentioned in #3352, if the license file is set and we're not in the root dir when calling cargo fmt it currently falls in a heap.

@gilescope
Copy link
Author

@calebcartwright I can attest to this feature being pretty broken at the moment so this should be an easy review - it can't make the feature any worse.

@gilescope
Copy link
Author

Can I help in any way or do anything more?

@calebcartwright
Copy link
Member

Can I help in any way or do anything more?

Not really. I appreciate the PR and that folks would probably prefer I get through issue and PR triage a bit more quickly 😄

However, the amount of work greatly overshadows the amount of bandwidth so the pace is what circumstances permit. I realize that this seems like a small/easy change, but path resolution changes have been a notorious source of bugs, often fixing one issue while creating another (especially on different platforms). It's not an area we have great tests for which ups the need for review/manual testing, and to be fully transparent, some of what I'll call rustfmt's auxiliary features (license checks, todo scans, etc.) are simply lower on the priority list relative to more core formatting issues.

Hmm but then I see dunce #4137 and then at some point it is removed... - trying to figure out why.

Probably best summarized in #3914 (comment), but that's part of the story in the review. There's been lots of various changes around path resolution (both for licenses/configs as well as loading out-of-line mods during the earlier parsing phases) made on the other branch that I'll need to cross reference.

Sorry I don't have any better news, but promise I'll get to it when I can!

@calebcartwright
Copy link
Member

@karyon - realize you're still in the early discovery phase but as you're reviewing the various non-backported changes from the 2.0 branch, this is one area I'd like to prioritize when time permits.

I think there were a few fixes as well as a different implementation that utilized some different dependencies. I'd love to get a better feel for if/how we should best backport those so that we can either get the underlying issues resolved or at least know whether we'd be setting up some gnarly merge conflicts if we were to press forward with a change like this prior to any backporting

@karyon
Copy link
Contributor

karyon commented Oct 26, 2021

@calebcartwright as far as I can see, there have been not many changes to config_type.rs or license.rs in the 2.0 branch, and none of them were large or looked relevant to me. So I think technically there shouldn't be major merge conflicts when backporting stuff. But I can't tell how much the backported features will indirectly interfere with this PR here :(

I searched for backportable path resolution PRs with this query. Here are the PRs that looked potentially relevant:

Does that help? Let me know what I should do!

@calebcartwright
Copy link
Member

Thanks @karyon, though none of those were what I was thinking of for this. I thought we had some other PRs that dealt with addressing config file resolutions in cases where there were multiple config files throughout the project. The dunce one is worth pursuing, and the others relate to some other areas that need attention, but I don't think of any of those will be applicable here.

If we are to go forward with this fix/enhancement to the license feature, I'd want to see some thorough test cases that cover the behavior. That should include scenarios with multiple config files with respective templates, as well as file-based definitions with command line overrides.

That being said, this is another config option, like report_todo and report_fixme, that just feels out of place for rustfmt. I understand the utility of the license check feature (and the other two as well), but I keep coming back to the conclusion those don't belong in rustfmt/would be better suited as a standalone tool.

@gilescope
Copy link
Author

gilescope commented Nov 5, 2021 via email

@calebcartwright
Copy link
Member

Just to be clear the licensing feature is pretty broken at the moment.

Thanks @gilescope, though to be honest I'm really not sure what to make of this given your prior comment to this same effect and the subsequent conversation.

To put it a little more pointedly, I'm seriously considering moving to drop this feature from rustfmt altogether, along with some of the other non-formatting related features that were stuffed into rustfmt for some reason.

However, if this is a feature we're going to keep, and if we're going to move forward with your proposed changes, then it would need to have some accompanying test cases to cover the various scenarios I described in #4967 (comment). I'm not requesting those tests at the moment though, as I wouldn't want you or anyone else to work on them in futility given the possible dropping/deprecation of this unstable feature.

@gilescope
Copy link
Author

gilescope commented Nov 6, 2021 via email

@calebcartwright
Copy link
Member

Thanks again for the PR and bearing with us while we contemplated the future of the option. I've decided we're going to move forward with the deprecation path so I'm going to close this.

Refs #5103

@gilescope gilescope deleted the license-relative-dir branch November 23, 2021 06:47
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

3 participants