search

vlachoudis / bCNC

GRBL CNC command sender, autoleveler and g-code editor
GNU General Public License v2.0
1.54k stars 528 forks source link

Refactor for codefactor #1773

Closed bgbsww closed 1 year ago

bgbsww commented 1 year ago

Obviously I'm new here. How do we feel about cleanup patches like this one, and if we like them, what granularity should they be? A method at a time, file at a time, subsystem at a time? Do we want advance discussion about the approach?

This PR is just for that discussion; I think it could be applied, but I'm not asking for it yet. Also, I'll get to see what codefactor thinks.

Harvie commented 1 year ago

Refactoring any library in /lib might not be that great idea. these are 3rd party libraries. there might be a new version released and we will want to upgrade them from upstream, therefore rolling back any improvements in the process. if you want to improve these, probably should get these improvements to the upstream first so they do not get overwritten when library gets updated.

bgbsww commented 1 year ago

Ok, thanks.

Harvie commented 1 year ago

But don't get me wrong. I think this is generaly a good idea. Upgrading these libraries to latest version to get the new fixes and sharing your improvements with the upstream so they can benefit as well.

bgbsww commented 1 year ago

Yep, I'll look into that. I just choose the file that codefactor complained the most about. I might look at some of the ones in project. Thanks for the clarification on /lib.

bgbsww commented 1 year ago

Hmm. So if I'm looking correctly, this specific file is a merger of items from two different upstreams, has had multiple local changes made, and is now very different compared to https://github.com/regebro/svg.path.

Is there a procedure for bringing in upstream changes? Or would we like this done differently to not modify the upstream code and make it easier?

Harvie commented 1 year ago

I beleive there were some local changes in some other PR. which is kinda annoying for this very same reason. Maybe you can find that and try to merge these to the upstream as well if they make sense :innocent: