Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
FEAT CLI conversion #249
FEAT CLI conversion #249
Changes from 42 commits
22c4c71
eacedf8
453728d
da6da30
a5abe50
5a9c0fc
b3a561e
1d01ff5
91d1b0f
7af9c26
8ad10cd
864d8ca
7589a06
b39ec14
9e76ee1
aa24050
02b3cf8
245197f
2b681ce
25b539c
50bf189
6ecb8cf
227f23c
426d663
c3b001c
37d3a87
9b9f342
6f5b71e
aa7c3ba
1c0352d
8d9b3c5
452fbdb
8150f9a
88e7be0
c38cb73
5233230
95a5ff3
a17b14b
7e63178
6e35770
02c95ad
c3491ef
7e072da
799f91b
1864f1c
8ddc6b9
bdfbf58
0e6b4cb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should instantiate the logger just once at the root of the module, and then just set
logger=logger
here. Is there any disadvantage to that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we could, although that would mean it gets initialised even if we import functions from the file to use elsewhere.
I don't think it would really impact much, I just personally dislike writing anything other than definitions at the top level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true. On the other hand it's now being initialized each time the function is called. Given that CLI usage consists of creating a new process anyway, it makes indeed no difference either way. If we ever get to a point where we use the logger elsewhere, we might have to rethink this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A more general question: I wonder if we should still document this, and similar cases, as "str or pathlib.Path", as users may not be familiar with
os.PathLike
. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly think
os.PathLike
is pretty self-documenting, but I could also see a user getting confused by not seeing the type they're expecting in the docstring.I'm happy with either way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to say if a user who doesn't know the type would guess that
str
is a path-like. Anyway, I won't object to leaving it as is, especially since it means that the type annotation and docstring are identical, I just wanted to bring up the possible friction here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of asking users to use
trusted=True
, we could also tell them the exact types to trust. WDYT?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think from a users' perspective, it might be nicer to give them the easiest fix possible.
When they're converting this file, and want to not deal, the log line above would've told them all of the untrusted types anyway, so if they wanted to go that route, they could just add those.
We could add a line along the lines of:
..., or set each of the unknown types listed previously to "trusted".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this touches on a more general point of how we should deal with
trusted=True
. I posted on discord about it, which is a better place to discuss than here :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, warning level logs would always be shown, even if I don't add
-v
. IMO, this should not be the case. So I would prefer that without-v
, the log level should beERROR
orCRITICAL
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the python docs:
The default level is WARNING, which means that only events of this level and above will be tracked, unless the logging package is configured to do otherwise.
I followed this because it's the default for python packages.
We can adjust the default level for our CLI, or we could also add a
-q, --quiet
option to reduce the log levelThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is true for logging in general, but for CLIs, there is the convention to not print anything if there is no issue. Ideally, I would like to avoid having a
--quiet
option, because it increases complexity and because it's unclear how it interacts with-v
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, good point.
I would raise that it could be considered an issue if untrusted types were present.
If a user had a file with untrusted types in it, and we converted it silently, they wouldn't know they needed to specify anything as trusted when using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this is more of an issue for the person loading the file, and they will know (unless they use
trusted=True
). Personally, I would not report it by default, but I can live with the current way.