search

markjoshwel / tomlantic

marrying pydantic models and tomlkit documents for data validated, style-preserving toml files
The Unlicense
1 stars 1 forks source link

Updated docstrings in tomlantic.py for issue 3 #6

Closed vlshields closed 8 months ago

vlshields commented 8 months ago

@markjoshwel After syncing my fork, following this tutorial, realized I should have merged the branch from my first PR with this new branch. Instead I just updated all the the relevant docstrings. I realized this is bad practice. I'm sorry, I'm still learning. I think I will just close the old PR go forward with this one.

If you have any tips or advice I would definitely appreciate that so much!

markjoshwel commented 8 months ago

@markjoshwel After syncing my fork, following this tutorial, realized I should have merged the branch from my first PR with this new branch. Instead I just updated all the the relevant docstrings. I realized this is bad practice. I'm sorry, I'm still learning. I think I will just close the old PR go forward with this one.

If you have any tips or advice I would definitely appreciate that so much!

regarding this, i'd suggest instead of making separate branches for each change, the workflow that i use and have seen others use is the following:

usually if an issue is too big for a single pull request, example being if it encompasses changes across a large chunk of a codebase, it would be split up into smaller issues. after being split up into smaller issues the same rule would apply (1 issue = 1 PR)

in the other pull request (#7) you have the same commit in this PR, and this is usually bad practice — each pull request should handle separate 'concerns' (or in this case, separate files!) as such, as #7 already has a commit that addresses this pull request, making this PR essentially redundant.

thankfully the issue that i opened is pretty small, so i could simply merge in this and your other PR without much hassle!

going forward as a contributor to open source projects (and thank you so much for being one!), i'd recommend you just make a single pull request for issues that encompass small changes like #3, which is exactly what you did with #7 (although i'm not sure if this was intended)

anyways, thank you so much for the contribution! and you needn't apologise for learning, we all are! if you have any follow-up questions, please feel free to continue the discussion here, even though i've closed all pertaining issues.

vlshields commented 8 months ago

in the other pull request (https://github.com/markjoshwel/tomlantic/pull/7) you have the same commit in this PR, and this is usually bad practice — each pull request should handle separate 'concerns' (or in this case, separate files!) as such, as https://github.com/markjoshwel/tomlantic/pull/7 already has a commit that addresses this pull request, making this PR essentially redundant.

Embarrassingly, I think I accidently created the branch for the README fixes from the the tomlantic.py fixes, rather than the main branch. I didn't notice until I already submitted the pull request from my fork, so I was afraid to mess things up :(.

Thank you for being so friendly and welcoming to beginners. I will eagerly await the next issue/assignment you need a hand with!!