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

package update, add missing dependency chardet #75

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

wangsha
Copy link

@wangsha wangsha commented Oct 2, 2021

No description provided.

@bryant-finney
Copy link
Collaborator

If these changes were merged, would it not cause a black version change on downstream projects?

Comment on lines +17 to +18
- python: '3.9'
script: python3 -m tox -e py39 -- --cov-report=xml
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice 👍

Pipfile Outdated
six = "~=1.12"
typing = {markers = "python_version<'3.7'", version = "~=3.7"}
pipfile = "*"
black = {markers = "python_version>='3.6'",version = "==21.9b0"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not leave black unpinned here?

Suggested change
black = {markers = "python_version>='3.6'",version = "==21.9b0"}
black = {markers = "python_version>='3.6'", version="*"}

Pinning It in the dev extras makes more sense to me

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I've noticed with black, with is quite frustrating, is that it cannot be unpinned without causing other problems. Pipenv will refuse to install it as a "pre-release" without adding the additional flag. This long traceback can be a bit off-putting for some users. Though easily resolved it could also break any scripts that use Pipenv and do not include this flag.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup I was able to confirm.

Additional information:

I'm interested if it's feasible to remove black as a minimal dependency. It seems like we could provide it as an extra in one way or another

Pipfile Outdated
Comment on lines 19 to 26
colorama = "*"
packaging = "*"
requirementslib = "*"
vistir = "*"
autopep8 = "*"
six = "*"
typing = "*"
chardet = "*"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where you able to check if any of these packages may have been pinned for a reason?

bryant-finney added a commit to bryant-finney/pipenv-setup that referenced this pull request Nov 7, 2021
still needs rules to run on scheduled/manual triggers

Signed-off-by: Bryant Finney <finneybp@gmail.com>
bryant-finney added a commit to bryant-finney/pipenv-setup that referenced this pull request Nov 7, 2021
Signed-off-by: Bryant Finney <finneybp@gmail.com>
bryant-finney added a commit to bryant-finney/pipenv-setup that referenced this pull request Nov 7, 2021
Signed-off-by: Bryant Finney <finneybp@gmail.com>
@bryant-finney
Copy link
Collaborator

oops, #75 in the commit messages is actually referring to #75 in my GitLab fork haha

bryant-finney added a commit to bryant-finney/pipenv-setup that referenced this pull request Nov 9, 2021
currently only installs `pyenv` and Python versions

Signed-off-by: Bryant Finney <finneybp@gmail.com>
bryant-finney added a commit to bryant-finney/pipenv-setup that referenced this pull request Nov 9, 2021
Signed-off-by: Bryant Finney <finneybp@gmail.com>
bryant-finney added a commit to bryant-finney/pipenv-setup that referenced this pull request Nov 9, 2021
Signed-off-by: Bryant Finney <finneybp@gmail.com>
bryant-finney added a commit to bryant-finney/pipenv-setup that referenced this pull request Nov 9, 2021
Signed-off-by: Bryant Finney <finneybp@gmail.com>
@Madoshakalaka
Copy link
Owner

The master branch now adds py38 39 310 support. The IDEA files actually have some value for intellij users, it defines a "file scope" for the source code, and a "file watcher" configuration to run black on the current file for example. Is there any rationale on using * for all the dependencies?

@Madoshakalaka
Copy link
Owner

I just checked and do find .idea a bit personal, it has a matt.xml that shows my vocabulary for the grammar checker for example. I'll remove it! Thanks for the suggestion!

@jshwi
Copy link
Contributor

jshwi commented Jul 7, 2022

@Madoshakalaka I definitely version .idea, and keep this in .idea/.gitignore

/dataSources.local.xml
/dataSources/
/httpRequests/
/shelf/
/sonarlint/
/tasks.xml
/usage.statistics.xml
/workspace.xml

This explains more, from Jetbrains:
https://intellij-support.jetbrains.com/hc/en-us/articles/206544839-How-to-manage-projects-under-Version-Control-Systems

Not sure about the <name>.xml, but I would imagine that's only relevant when running under <name>, so don't think it would hurt to have array of <name>s for individual contributors, personally

@Madoshakalaka
Copy link
Owner

@jshwi thanks, that makes sense. I learned more about .idea files despite being a 5 year intellij user :)

@jshwi
Copy link
Contributor

jshwi commented Jul 8, 2022

@Madoshakalaka Oh I had another read, here's what they say about dictionaries:

user dictionaries folder (to avoid conflicts if other developer has the same name)

Funny, sometimes the file gets created for me, sometimes it doesn't

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.

4 participants