Skip to content

Conversation

saba8814
Copy link

@saba8814 saba8814 commented Oct 10, 2025

Description

Contribution for:

test: refactor Furl() as a plain function, curl(url), in `test/infamy/util.py

Checklist

Tick relevant boxes, this PR is-a or has-a:

  • Bugfix
    • Regression tests
    • ChangeLog updates (for next release)
  • Feature
    • YANG model change => revision updated?
    • Regression tests added?
    • ChangeLog updates (for next release)
    • Documentation added?
  • Test changes
    • Checked in changed Readme.adoc (make test-spec)
    • Added new test to group Readme.adoc and yaml file
  • Code style update (formatting, renaming)
  • Refactoring (please detail in commit messages)
  • Build related changes
  • Documentation content changes
    • ChangeLog updated (for major changes)
  • Other (please describe):

@troglobit
Copy link
Contributor

troglobit commented Oct 13, 2025

Imo it looks good, but I think you should rebase and squash out the merge commit and the 'add missing import' at least. We try to keep commits and git history clean, most of us follow the DCO and add a "signed-off-by" to the commit message, and these days we also require GPG signing your commits.

Btw, commits should be "bisectable" — meaning that every commit should build and tests should pass. So if I read your changes correctly maybe your two main commits should be squashed into one, because it looks like the tests would fail if you only built and tested with the furl removal commit?

You can read more about our contributing guidelines here: https://github.com/kernelkit/infix/blob/main/.github/CONTRIBUTING.md (I just noticed that GPG signing was missing, but we'll add that soon.)

@saba8814 saba8814 marked this pull request as draft October 13, 2025 16:19
@saba8814 saba8814 force-pushed the furl-refactoring branch 2 times, most recently from 9c93ced to e2526e5 Compare October 13, 2025 16:20
@mattiaswal
Copy link
Contributor

Maybe we should take this in mind as well psf/requests#6735 and refactor my workarounds out of restconf.py

@saba8814 saba8814 marked this pull request as ready for review October 13, 2025 17:05
@saba8814
Copy link
Author

Imo it looks good, but I think you should rebase and squash out the merge commit and the 'add missing import' at least. We try to keep commits and git history clean, most of us follow the DCO and add a "signed-off-by" to the commit message, and these days we also require GPG signing your commits.

Btw, commits should be "bisectable" — meaning that every commit should build and tests should pass. So if I read your changes correctly maybe your two main commits should be squashed into one, because it looks like the tests would fail if you only built and tested with the furl removal commit?

You can read more about our contributing guidelines here: https://github.com/kernelkit/infix/blob/main/.github/CONTRIBUTING.md (I just noticed that GPG signing was missing, but we'll add that soon.)

Think I have adressed all the concerns from above.

@saba8814
Copy link
Author

Maybe we should take this in mind as well psf/requests#6735 and refactor my workarounds out of restconf.py

I would prefer that to be a separate issue since I am still getting to know my way around in Infix.

@troglobit
Copy link
Contributor

Maybe we should take this in mind as well psf/requests#6735 and refactor my workarounds out of restconf.py

Is definitely time for it, but maybe we should do that in a separate PR? Meanwhile you could write it down in an issue so we don't forget it 😏

@saba8814
Copy link
Author

Maybe we should take this in mind as well psf/requests#6735 and refactor my workarounds out of restconf.py

Is definitely time for it, but maybe we should do that in a separate PR? Meanwhile you could write it down in an issue so we don't forget it 😏

Fell free to put that one on me when you've formulated the issue.

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.

test: refactor Furl() as a plain function, curl(url), in `test/infamy/util.py

3 participants