-
-
Notifications
You must be signed in to change notification settings - Fork 6
Feedback #1
Comments
It seems I have to use https://github.com/prettier/pre-commit/runs/953985278?check_suite_focus=true#step:8:24 |
I'm going AFK for a while. |
In fact, it seems to be the way to go. |
I've tested it, and it works like a charm. I've created a simple PR (#2) to make sure the versions in the different files don't go out of sync ; this will be helpful if you choose to go for One thing to keep in mind, is that you will also need to keep manually creating tags when a new version is release ; something that seem to be also handled by the github action above. For instance, the current commit on master can be / should be tagged Also for the tests failing on windows... I have no ideas ; I guess it's an error on pre-commit's side. |
Thanks for testing, the reason I don't want |
Thanks for the PR, but I still want Prettier version in just one place(package.json). I have another idea, I'll try later. |
@FloChehab What do you think the idea adding another bin file? #4 |
Looks good to me ; and it works perfectly. |
Thank you! |
@fisker, Once you consider this repo done, could you create a tag |
I created a tag What if we found bug, and pretter haven't release a new version? |
The main reason I did't choose auto-update is some new version of Prettier might require changes, most likely we will ship |
On a second thought, I might made mistake by using If we use But current solution I use should not able to do that. I'm not sure, but I'll keep this in mind. |
I think it will be ok to update the tag and make it point to another commit ( |
I thought it would be possible to override with - repo: https://github.com/prettier/pre-commit
rev: "v1.0.0"
hooks:
- id: prettier
args: ['--version']
additional_dependencies: ["prettier@2.0.4"] But it's not working (it shows So I guess, moving tags is ok. |
Wait, you are tesing on |
Well, it works on this commit ! I do get 2.0.4 printed. |
But on this commit, you don't have prettier version specified in https://github.com/prettier/pre-commit/blob/44ca7e9574378f14274107ab1c72d1eb1cced843/package.json |
This commit has pre-commit/.pre-commit-hooks.yaml Lines 20 to 21 in 44ca7e9
So I guess we still need use this property. |
There is a choice to be made. I personally don't have a strong opinion on this matter. |
@FloChehab Can it done by branch? we can create branch for every version. |
Yes, Maybe we can get the opinion of pre-commit's maintainer on what we are doing here (started here) ; as he is the one who added pre-commit support to prettier's main repo 3 years ago in prettier/prettier#2414 (@asottile I am trully sorry to bother you, but if you have a bit of spare time, please have a look at this.) |
All branches https://github.com/prettier/pre-commit/branches and tags https://github.com/prettier/pre-commit/tags created. |
Hi, didn't read the whole thread but I'm not sure this is the best thing to do here -- if you've set up caching it doesn't really make a difference and the extra repository is likely to lead to more confusion and indirection. That said, I can help you get this set up if you want and describe some of the reasons / downsides for the mirrors you see above were made while the project was in more of its infancy and upstreams were not as receptive to taking the pre-commit configuration (some of the reasoning for this was the unfortunate naming of the yaml file, that's at least been fixed). The mirror repositories aimed to have slightly more compatibility than your typical repo and used If you can find a way that works with I think ideally if we could collaborate on those ideas there and get something working with I think the questions above were about pre-commit's preference for the (unrelated but I saw it while scrolling through the other thread) The usage of Lastly, once we're done with this, we'll want to update the reference to prettier on pre-commit.com -- that can be done with a PR to https://github.com/pre-commit/pre-commit.com |
First of all, thanks for the quick reponse.
The main reason, we decide to use extra repo is, people will get unstable prettier from github. Npm (stable) version should prefered.
I do found one solution to make it work with But I decided to revert it, reason #1 (comment) . I thought With the solution #4, people won't be able to override Prettier. |
Usually they'll control the |
I still prefer not |
makes sense! |
No, |
@asottile As I understand, pre-commit do |
it isn't currently, the only place that pre-commit currently searches is the in older versions of npm it was possible to trigger an |
Is this |
it's the true npm prefix dir, it's set up as an isolated environment using nodeenv |
Wow, I thought that was only for |
This also means, if I use the one from |
Understanding this makes me think #4 is actually a good solution for now, it won't override global one, |
no no, that's not how it works -- it makes entirely isolated environments :) |
This make sense. |
What do you think the idea of searching more places? That's how |
you mean like |
Yes, that's what I mean. |
Just two side notes after reading your discussion:
|
Hi @fisker, sorry I forgot to come back to this issue. So should we consider the current setup as final ? |
I think so. |
No description provided.
The text was updated successfully, but these errors were encountered: