search

crystal-lang / shards

Dependency manager for the Crystal language
Other
763 stars 100 forks source link

Add macOS nightlies to CI #464

Closed Sija closed 3 years ago

Sija commented 3 years ago

Refactored some parts as well like include-ing windows in the matrix instead of having it there from start - especially apt as windows support is still in-the-works.

Sija commented 3 years ago

This could be merged...

Sija commented 3 years ago

@bcardiff Could you please review this?

bcardiff commented 3 years ago

Is there a reason to exclude windows from using crystal latest?

Sija commented 3 years ago

@bcardiff yes, it doesn't work. for more detailed explanation please ask @oprypin.

oprypin commented 3 years ago

And the reason for that is https://github.com/crystal-lang/distribution-scripts/issues/64#issuecomment-778775239

oprypin commented 3 years ago

@Sija Why do you have to change the whole file's formatting to adhere to your liking?

Sija commented 3 years ago

@oprypin Whole file? Don't exaggerate, adding some spaces for readability and changing name of one job is not whole file. What's wrong with those changes anyway?

asterite commented 3 years ago

@Sija They are irrelevant to the change you want to make.

If you want to change someone else's style, I think that should be discussed and done separately.

Code is written by people if you go and change that when it's not needed, while also adding additional changes, the ones that wrote the original code might think "What's your problem with my style? BAM. I don't like your PR."

Not that that will effectively happen. But at least in my case when I contribute to a project I tend to avoid changing style things if they have nothing to do with the PR's goal.

asterite commented 3 years ago

They also make reviewing the PR harder! "Why was this changed? Is this relevant? I don't understand."