search

rdp / os

The OS gem allows for some easy telling if you’re on windows or not. OS.windows? as well as some other helper utilities
MIT License
145 stars 33 forks source link

Top level should be module not class #27

Open ioquatix opened 7 years ago

ioquatix commented 7 years ago

Hello

The top level of your gem should be a module, not a class, to follow standard conventions.

rdp commented 7 years ago

PR's welcome! :)

On Tue, May 16, 2017 at 8:22 PM, Samuel Williams notifications@github.com wrote:

Hello

The top level of your gem should be a module, not a class, to follow standard conventions.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rdp/os/issues/27, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAw0MlLKM1QEg04XGMkj5c5e6t-kC1iks5r6lnMgaJpZM4NdS_l .

pboling commented 2 years ago

@ioquatix is correct, a class at the top-level can cause a number of problems, and is against standard convention for good reasons. I've made the mistake before, and vowed never to repeat it.

Reverse Dependencies

This gem has many reverse dependencies, totalling hundreds of millions of downloads, and, given the nature of software, many of those mature, stable packages would have little reason to be updated other than to pull in a major version upgrade to this gem.

This gem should not make a change from top-level class to top-level module without a major version bump, as it would be an "incompatible API change" IMO. If it were to happen it would result in "chained upgrade hell", fracturing the Ruby ecosystem a bit like is happening right now between Faraday v1 and Faraday v2.

A Version Split?

In a version split a project's dependencies either need to all be updated to work with v.<next>, or they all have to remain stuck at v.<current> (forever). Many dependencies are not being maintained any more which has resulted in a great deal of effort expended on the part of too few people.

As a taste of why a version split outcome is undesireable, here are some projects affected by the Faraday version split: elasticsearch, discourse, oauth2, danger, and all their dependencies. Danger, used by thousands of projects, is at risk, as many major projects are removing it so they can upgrade to Faraday 2, including Github's oktokit and Slack's ruby client.

What is the state of this library?

As an outside observer who has never before interacted with it, or even used it...

Failures:

1) OS has working cpu count method Failure/Error: assert (cpu_count & (cpu_count - 1)) == 0 # CPU count is normally a power of 2 Test::Unit::AssertionFailedError:

is not true. # ./spec/os_spec.rb:124:in `block (2 levels) in ' 2) OS should provide a path to directory for application config Failure/Error: assert OS.app_config_path('appname') == '/home/xdg/Library/Application Support/appname' Test::Unit::AssertionFailedError: is not true. # ./spec/os_spec.rb:150:in `block (2 levels) in ' Finished in 0.03682 seconds 36 examples, 2 failures ``` - #67 - Test suite hasn't been upgraded to RSpec 3 ``` An error occurred while loading ./spec/os_spec.rb. Failure/Error: config.expect_with :rspec, :stdlib # enable `should` OR `assert` ArgumentError: :stdlib is not supported # ./spec/spec_helper.rb:8:in `block in ' # ./spec/spec_helper.rb:7:in `' # ./spec/os_spec.rb:1:in `require' # ./spec/os_spec.rb:1:in `' ``` - #70 - Test suite needs to be upgraded to modern [rspec-autotest](https://github.com/rspec/rspec-autotest) # Path Forward Given the state of this library overall I think the best route forward would be to create a new gem with the same API and funtionality, but with a new name and namespace, so that unmaintained libraries that rely on `os` continue to work without introducing a "chained upgrade hell". To that end, I have [forked this library](https://github.com/pboling/moss), renamed the fork, and will soon release a new gem called `moss` (Mossy Operating System Stuff) with a top-level namespace module of `Moss`, which provides the same API as this gem. It will be based on the modern `bundler gem` template, use github actions, integrate the stability fixes from [#48](https://github.com/rdp/os/pull/48), and have [code coverage reporting built in](https://dev.to/pboling/ippccr-in-pursuit-of-perfect-code-coverage-reporting-3a2i) to pull requests, etc. Because it will be a new gem and a new namespace, the two gems can be used side by side with no problems! No one is forced to upgrade anything, and the ecosystem remains intact! Prior to making the fork forky though (the namespace change) I'll do upgrades that I can PR upstream to this repo, so they both benefit! I will upstream fixes for: - [x] #55 fixed by #59 - [ ] #56 - [ ] #57 - [ ] #58 - [x] #60 fixed by #59 - [ ] #61 - [ ] #62 - [ ] #63 - [x] #64 fixed by #72 - [x] #67 fixed by #68 - [x] #70 fixed by #71 # Q & A Question: Why am I doing this if I have never before used this library? > I am working on upgrading my `rspec-pending_for` [gem](https://github.com/pboling/rspec-pending_for), and need the functionality this library provides, and will probably want to extend the functionality. Given that the maintainer may not want my extensions, the trouble caused by churn if the issues were to be addressed, and the actual current issues it seemed best to hard fork. - @pboling, here, now Question: Why `moss`? > I considered `oss`, but it has a strong association with Open Source Software already. I considered `os2`, but google searches for os2 in the computer programming context are saturated by IBM's eponymous product. So aping on the _stolid_ linux tradition of package names that are self-referencing, I chose `moss`. - @pboling, here, now Interested to hear what @rdp thinks about all this.
ioquatix commented 2 years ago

I think forking should almost always be a last resort.

pboling commented 2 years ago

I normally agree, but in this case it might be quite disruptive to fix the top-level namespace. What I've been dealing with on the faraday version split is real bad. I maintain the oauth2 gem, and I ended up having to remove danger in order to support faraday v2. oauth2 has a huge reverse dependency tree as well. It was a mess.

On the other hand, when factory_girl became factory_bot, a hard fork, it was easy.

ioquatix commented 2 years ago

Yeah fair enough.

pboling commented 2 years ago

I'd be happy to PR the hard fork (by which I mean a namespace change and gem name change) into this repo if @rdp agrees to change both. Then it would be just like the factory_girl/bot rename. Alternatively, or in addition, would be happy to take over maintenance.