Closed malligaraj closed 2 years ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
@muellerzr With this change I ran nbdev_build_docs
locally to see if that works and that modified the file docs/data.external.html
.
I am not sure if I am supposed to push that .html
file. The checks seem to pass without that file. Should I push that .html
file?
Yes! Please do 😄 we don't have a test for that as in the real fastai library Jeremy likes to generate and preview that himself. But here we want to push the HTML
Honestly may try and add a check for that. We'll see :smile:
@malligaraj taking a small close look at this, but a few of us will be adding more comments throughout the day!
One small formatting nit, which wasn't discussed in the style guide and I need to do so.
When adding docments
, make sure to have just one spaces behind the variable, rather than trying to prettily align them all. Over time refactoring makes it harder to keep pretty, so it's better to just keep to a simple format, such as:
def func(
a:int, # my docment string
):
...
Hi @malligaraj. Thanks for the first PR for this documentation sprint. In addition to the documentation additions, your PR has been quite helpful as it has shown us there are edge cases we need to resolve in our style guide.
tyoc213 and muellerzr have already provided good feedback in their reviews, so one more comment on the code itself: where applicable add return types. For example,
def fastai_cfg() -> Config: # A Config object for `fastai`
or
def untar_data(
...
) -> Path: # Path to extracted file
For bookkeeping, this PR is for issue #9.
@malligaraj taking a small close look at this, but a few of us will be adding more comments throughout the day!
One small formatting nit, which wasn't discussed in the style guide and I need to do so.
When adding
docments
, make sure to have just one spaces behind the variable, rather than trying to prettily align them all. Over time refactoring makes it harder to keep pretty, so it's better to just keep to a simple format, such as:def func( a:int, # my docment string ): ...
@muellerzr I have fixed them. Got a little confused after looking at an example in https://fastcore.fast.ai/docments
Hindsight, I should have just stuck to fastai docment Sprint Style Guide
Hi @malligaraj. Thanks for the first PR for this documentation sprint. In addition to the documentation additions, your PR has been quite helpful as it has shown us there are edge cases we need to resolve in our style guide.
tyoc213 and muellerzr have already provided good feedback in their reviews, so one more comment on the code itself: where applicable add return types. For example,
def fastai_cfg() -> Config: # A Config object for `fastai`
or
def untar_data( ... ) -> Path: # Path to extracted file
For bookkeeping, this PR is for issue #9.
@warner-benjamin No problem :relaxed: . Thank you for setting up this sprint, it is very helpful for me :pray:
I have added the return types
Thanks @malligaraj. BTW, any reason why we're doing so many force pushes? You should only have to do those in very extreme circumstances because it can mess with the commit history
Oh, I thought it might be easier to merge the PR if it was a single commit. Will read up more on Pull Requests.
I will stop squashing and force pushing my commits. Thank you for the tip :+1:
The doc string for c_key
in all the methods isn't quite right, as c_key
is the dictionary key for fastai_cfg
, not the folder itself.
c_key='archive' # Name of a folder in `fastai_cfg` to save the file. Should be one of [`'data', 'archive', 'storage', 'model'`]
Add docment to data.external. This a PR for issue #9