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

add poetry.lock as toml #280

Merged
merged 1 commit into from Feb 9, 2022
Merged

add poetry.lock as toml #280

merged 1 commit into from Feb 9, 2022

Conversation

jvllmr
Copy link
Contributor

@jvllmr jvllmr commented Feb 5, 2022

I noticed that the lock files from poetry and yarn aren't in here yet. Poetry uses toml. Yarn is another story. Yarn v1 files aren't valid yaml files while Yarn v2 lock files are. I think it's better to go just with a text file here.

'pylintrc': EXTENSIONS['ini'] | {'pylintrc'},
'README': EXTENSIONS['txt'],
'Rakefile': EXTENSIONS['rb'],
'setup.cfg': EXTENSIONS['ini'],
'WORKSPACE': EXTENSIONS['bzl'],
'wscript': EXTENSIONS['py'],
'yarn.lock': {'text'},
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't improve the identification -- it will get text already

Copy link
Contributor Author

@jvllmr jvllmr Feb 5, 2022

Choose a reason for hiding this comment

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

I oriented this change on the Gemfile.lock entry:

'Gemfile.lock': {'text'},

What exactly is the difference in the cases between Gemfile.lock and yarn.lock?

Furthermore, checking yarn.lock with the cli returns no result:

identify /home/janv/yarn.lock 
identify: no decode delegate for this image format `LOCK' @ error/constitute.c/ReadImage/572.

But it does after the change:

identify /home/janv/yarn.lock
["file", "non-executable", "text"]

Copy link
Member

Choose a reason for hiding this comment

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

your output is doctored, you're running imagemagick identify

the Gemfile.lock is a mistake, it seems -- it should have a more specific tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh... my bad. I had trouble getting the cli to run and got something wrong somehwhere. How about adding a __main__.py so the cli can be run with python -m identify ? I will revert the yarn.lock change.

Copy link
Member

Choose a reason for hiding this comment

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

I mean you've kinda killed my trust -- why did you make up output?

the command is called identify-cli to disambiguate with imagemagick

Copy link
Contributor Author

@jvllmr jvllmr Feb 5, 2022

Choose a reason for hiding this comment

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

I did the stupid mistake to not read the docs/README. I know this isn't trustworthy and I want to apologize for that. I was so sure that identify was the right cli command so I stripped off the python -m before it. I didn't pay enough attention to what I was doing. Lesson learned.

However, I want to regain your trust by pointing out the code where poetry.lock is interpreted as a toml file:
https://github.com/python-poetry/poetry/blob/31657e7e5c47da758c14645035e59eb7813ac2f8/src/poetry/packages/locker.py#L53

Again, sorry for killing your trust. This was embarrassing.

@jvllmr jvllmr requested a review from asottile February 5, 2022 14:35
@jvllmr jvllmr changed the title add names poetry.lock & yarn.lock add name poetry.lock Feb 5, 2022
@asottile asottile changed the title add name poetry.lock add poetry.lock as toml Feb 9, 2022
Copy link
Member

@asottile asottile left a comment

Choose a reason for hiding this comment

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

@asottile asottile merged commit 8acfb99 into pre-commit:master Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants