Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A word of caution from Norm #13

Closed
benjub opened this issue Dec 28, 2021 · 2 comments · Fixed by #42
Closed

A word of caution from Norm #13

benjub opened this issue Dec 28, 2021 · 2 comments · Fixed by #42

Comments

@benjub
Copy link
Collaborator

benjub commented Dec 28, 2021

Digging through my older emails, I found an email I received from Norm in 2017, from which I extract the following word of caution. It was before the move to GitHub, but some parts are still relevant. This is not a real "Issue" in GitHub's sense, but I thought I'd leave it open here for a while.

Another issue with the metamath program that people have requested is putting it on GitHub. However, strange as it may seem I actually don't want to encourage major contributions at this point in time. Almost every unsolicited patch I've been sent has had to be rewritten by me to fix memory leaks and other issues, and it can take a lot of my time to analyze them in complete detail. C is a dangerous language if you don't have intimate knowledge of all details of the program's data structures etc. If I put it on GitHub, I would dread having someone make a massive change that would require a week or more of my time to analyze. Ensuring bug-free code can take more time than it took to write it. I keep past versions archived and will put them on GitHub when that day comes.

@wlammen
Copy link
Contributor

wlammen commented Dec 28, 2021

I haven't finished the review of PR #9 yet, but I can comment on my experience so far.

  1. The source (as of 0.198) is not in the shape of being easily maintained by a single person, let alone a team of developers all having their own agenda in mind. The first step imho should be documentation. All the implicit rules, coding styles, architecture need to be written down, so that they can be followed, at least in principle.
  2. The source should then be refactored to follow common design principles. This is not the case. Both on the architecture and detail level there are lots of deficiencies. I wouldn't say, it is all messy, Norm was clearly trained in structural thinking enough to avoid that. But it was not written by a professional either.
  3. Only after having done that, one should allow extensions.

I can confirm that doing a review is exhausting. I already spent several hours on that in PR #9 . Occasionally I review code in my company, but there I know the code base and the developers. For those interested in general aspects of review, visit http://users.ece.cmu.edu/~koopman/pubs/code_review_checklist_v1_01.pdf or https://www.evoketechnologies.com/blog/simple-effective-code-review-tips .

@jkingdon
Copy link
Collaborator

Maybe we should put some excerpts from that paragraph from Norm in the CONTRIBUTING or whereever we talk about coding standards and the like? Along with a statement along the lines of "I agree that we should try to migrate essential functionalities to metamath-knife eventually, but it will take a while to reach parity. I don't think we should significantly change the user-visible behavior of metamath.exe though" from metamath/set.mm#2392 (comment) (or however strongly or weakly we want to say such a thing - I'm not completely sure where to draw the line but I would tend to think of metamath.exe as being in maintenance mode rather than as being something we encourage as a place to implement new functionality).

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 a pull request may close this issue.

3 participants