search

hunyadi / md2conf

Publish Markdown files to Confluence wiki
MIT License
56 stars 31 forks source link

Extract test page id to a sep constant, use Pathlib #36

Closed vladistan closed 6 months ago

vladistan commented 6 months ago

Extracted the test page ID to a constant and put it at the top of the file. This makes it easier to for other devs to work on this tool by modifying the code just in one place

Also, switched to Path lib from os.path as it is a more modern alternative and it has been a part of Python standard lib for a while. Probably should do it in the main code as well, but need to get more familiar with it first

hunyadi commented 6 months ago

That's definitely a simple but nevertheless great improvement, thanks for contributing!

As for pathlib, I would definitely prefer switching everywhere (or not at all), which would help avoid weird patterns like

synchronize(str(self.sample_dir / "example.md"))

in which there is no real need to convert to a str but instead we could pass a pathlib.Path directly. Using a single path formalism across the entire code base would also make it easier for readers to understand.

vladistan commented 6 months ago

Yes, this is just a first step into this direction. I just wanted to get a fell how receptive you'd be to PRs as some repo maintainers do not merge PRs for years.

If you think you'd benefit from it I can go through the rest of the codebase and patch it up

On Fri, May 31, 2024 at 12:35 PM Levente Hunyadi @.***> wrote:

That's definitely a simple but nevertheless great improvement, thanks for contributing!

As for pathlib, I would definitely prefer switching everywhere (or not at all), which would help avoid weird patterns like

synchronize(str(self.sample_dir / "example.md"))

in which there is no real need to convert to a str but instead we could pass a pathlib.Path directly. Using a single path formalism across the entire code base would also make it easier for readers to understand.

— Reply to this email directly, view it on GitHub https://github.com/hunyadi/md2conf/pull/36#issuecomment-2142615341, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAI3IMTJCI5QNZIHGTPOGDZFCRDRAVCNFSM6AAAAABISPHFLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBSGYYTKMZUGE . You are receiving this because you authored the thread.Message ID: @.***>

hunyadi commented 6 months ago

If the pull request is focused and succinct (like yours), I am happy to merge. What I find more challenging is to merge pull requests that introduce incomplete features, or new features that pertain to specific use cases but impact the overall structure of the code. It's typically much harder to verify in these cases that the improvements don't break or adversely impact existing functionality.

vladistan commented 6 months ago

Yes makes sense.

On Fri, May 31, 2024 at 1:52 PM, Levente Hunyadi @.***> wrote:

If the pull request is focused and succinct (like yours), I am happy to merge. What I find more challenging is to merge pull requests that introduce incomplete features, or new features that pertain to specific use cases but impact the overall structure of the code. It's typically much harder to verify in these cases that the improvements don't break or adversely impact existing functionality.

— Reply to this email directly, view it on GitHub https://github.com/hunyadi/md2conf/pull/36#issuecomment-2142725840, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAI3IOVXGRBWB4NCTS7X7LZFC2GPAVCNFSM6AAAAABISPHFLSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBSG4ZDKOBUGA . You are receiving this because you authored the thread.Message ID: @.***>

vladistan commented 6 months ago

Closing in favor of https://github.com/hunyadi/md2conf/pull/37