ninech
Housekeeping
Thank you @n-rodriguez for putting this together and for investigating and reaching out to Netbox core about the nature of that collections URLs error. We also just encountered that error.
Hopefully this can be merged soon, things have been stable for so long this repo hasn't needed much maintenance.
Hey @n-rodriguez, first thank you very much for the effort!
I'm quite reluctant on merging the whole MR if I'm honest. I'd have preferred these changes to be split up in multiple PR's. As it's easier to review and also allows a conversation about the proposed changes.
For example:
- https://github.com/ninech/netbox-client-ruby/pull/70/commits/db08e3b63c5743dcc79f4deac8ace7804b77340d should probably be merged quickly, as it's an actual fix.
- In terms of formatting, I'd prefer to use standard instead of a custom Rubocop config. I also prefer a linter that checks only changed files, so we don't change a ton of files just because we use a new formatter, but rather update them to the new style if they are touched because of something else.
- Binstubs: Personally, I don't really use them. They are a personal preference, so I'd prefer to exclude them from Git.
- CI: I like the improvements here, but would also prefer them in a separate PR.
Would you be down splitting these changes up? Otherwise I can offer to do that, but the time I can invest in the gem is quite limited. So no guarantees on the timeline :). Thank you again for the effort!
I'd have preferred these changes to be split up in multiple PR's.
I can do that but the question is : will you merge rubocop related changes? some are cosmetic changes, others are performance related?
See:
- https://github.com/ninech/netbox-client-ruby/pull/70/commits/1403db71806fb9707196aeda7cf69be9f1314baf
- https://github.com/ninech/netbox-client-ruby/pull/70/commits/62012968c8cb91aee8fdea322a50c27f94faef23
- https://github.com/ninech/netbox-client-ruby/pull/70/commits/33afbaa3371cd83db1a03b3170c6428b685cb03b
- https://github.com/ninech/netbox-client-ruby/pull/70/commits/f89903320f5801fafe66e0bfa6b0d0c2dee51c44
This one is about 'return in void context' which is significant :
- https://github.com/ninech/netbox-client-ruby/pull/70/commits/e1c4db562979a826762d443b5be1804faf9a5602
I suggest we merge this commits :
| Commit | Reason | Link |
|---|---|---|
| Update gems | to have a recent dev environment | https://github.com/ninech/netbox-client-ruby/pull/70/commits/e839920d403fedfbd9b4a6ac6fc6cc5f86d4e48d |
| Disable RSpec monkey patching | considered as best pratices | https://github.com/ninech/netbox-client-ruby/pull/70/commits/bef6a90b6112eafda31474c9d7a567a30533466c |
| Don't reopen Ruby modules in spec | considered as bad practice | https://github.com/ninech/netbox-client-ruby/pull/70/commits/a180a6345ac6a465bb0a501402f50965b1bc7b53 |
| Move development gems in Gemfile | considered as best pratices | https://github.com/ninech/netbox-client-ruby/pull/70/commits/238453356f3524b2fd65f6ccc64bfa3ed7e8bc1b |
| Cleanup gemspec | following best pratices | https://github.com/ninech/netbox-client-ruby/pull/70/commits/ccc6b5770c05480c9dde736d3479dadaedaf6d0c |
| Bump to latest Rubocop version | it was here before my PRs so update it, we can switch to standard gem in an other PR | https://github.com/ninech/netbox-client-ruby/pull/70/commits/0088307a1556a6fba872237c5de542cec929fb25 |
| Coding style: add missing frozen string literal comment | considered as best pratices, anticipate Ruby 3.4, see https://gist.github.com/fxn/bf4eed2505c76f4fca03ab48c43adc72 | https://github.com/ninech/netbox-client-ruby/pull/70/commits/11bda096f1a63961fb0909700e4aa89d52158a9c |
| Drop support of Ruby 2.7, add support of Ruby 3.3 | Ruby 2.7 is EOL since Mar 30, 2023 | https://github.com/ninech/netbox-client-ruby/pull/70/commits/32d601ac191253d59de14eee422432dd72e32b71 |
| Fix specs: JSON middleware was introduced in v1.10.0 | a fix | https://github.com/ninech/netbox-client-ruby/pull/70/commits/6fd22ba414a0e1ccf84c70c53e3eb616a4519e4b |
| Use appraisal gem to manage CI gemfiles | it's easier to run tests locally | https://github.com/ninech/netbox-client-ruby/pull/70/commits/b767b89b8410b4acf6c4b98347d1a1d2102c4a27 |
| Update GithubActions config | a fix | https://github.com/ninech/netbox-client-ruby/pull/70/commits/45a6db4f50267cf1f8898e16bd7368b7a5709107 |
| Install simplecov gem to have code coverage | to have code coverage | https://github.com/ninech/netbox-client-ruby/pull/70/commits/faac5e54fc794c7f697c825a18e61b2ebba8bd71 |
| Fix collections urls | a fix | https://github.com/ninech/netbox-client-ruby/pull/70/commits/db08e3b63c5743dcc79f4deac8ace7804b77340d |
| Coding style: fix Performance/StringIdentifierArgument offenses | performance | https://github.com/ninech/netbox-client-ruby/commit/1403db71806fb9707196aeda7cf69be9f1314baf |
| Coding style: fix Lint/SymbolConversion offenses | performance | https://github.com/ninech/netbox-client-ruby/commit/62012968c8cb91aee8fdea322a50c27f94faef23 |
| Coding style: fix Style/HashEachMethods offenses | performance | https://github.com/ninech/netbox-client-ruby/commit/33afbaa3371cd83db1a03b3170c6428b685cb03b |
| Coding style: fix Lint/ParenthesesAsGroupedExpression offense | https://rubystyle.guide/#parens-no-spaces | https://github.com/ninech/netbox-client-ruby/commit/f89903320f5801fafe66e0bfa6b0d0c2dee51c44 |
| Coding style: fix Lint/ReturnInVoidContext offenses | https://docs.rubocop.org/rubocop/cops_lint.html#lintreturninvoidcontext | https://github.com/ninech/netbox-client-ruby/commit/e1c4db562979a826762d443b5be1804faf9a5602 |
the time I can invest in the gem is quite limited.
me too. that's why I'm not sure it's the right way to go