BNFC / bnfc

BNF Converter
http://bnfc.digitalgrammars.com/
587 stars 165 forks source link

Simplify code #443

Closed ivanperez-keera closed 1 year ago

ivanperez-keera commented 1 year ago

Simplify code by removing redundant elements (e.g., brackets, $), replacing several compositions with (faster) standard functions intended to have the same effect, etc.

ivanperez-keera commented 1 year ago

@andreasabel I saw these opportunities and just sent a PR. I tried to justify the changes in sufficient detail in the commit messages. Let me know if you want me to change anything.

ivanperez-keera commented 1 year ago

@ivanperez-keera Sorry for the slow response, which was due to my hesitation.

No worries.

Most importantly, I don't know how such a large-scale cosmetic operation will affect the pending PRs. It might introduce extra conflicts to be resolved.

I understand.

Secondly, I sometimes have e.g. redundant $ for uniformity, something that hlint cannot understand.

I thought that could be the case. But I also found cases where that wasn't happening, so I thought it wasn't a hard rule. If you are happy with some commits but not others, maybe you can drop the commits you don't like. I isolated the kinds of changes for easier processing on your side.

So, I am bit hesitant to invite hlint in. In my experience, it is not a productivity gain, as it requires a lot of effort if you want to have exceptions from hlints rules in some cases.

I understand. I was reviewing the code and saw things that I would simplify. (I did eventually use hlint to find more such things, but for me the point was code cleaning & simplification, not just hlinting code. These are changes I would make even if hlint didn't recommend it.)

May I ask why you are sending this PR? Are you planning to do major work on BNFC?

I don't know yet. At this point I'm investigating the code. While I was reading, i saw cleaning opportunities, so I just sent a PR.

There's a few big features I'd like to see in a parser generator and language analysis tool. We (me and some past colleagues) discussed the possibility of revamping a parser generator with similar ideas to BNFC that we built in 2003-2010, but I thought perhaps it would be better to add those features to BNFC. IMO it's better if we push together in a common direction. That's all I can say for now.

ivanperez-keera commented 1 year ago

@andreasabel I took a look around the commit of removing $ explicitly and I found that many other cases where I removed $ (even some you did not bring up in your review) would fall in the same pattern that you pointed out. Shall I just drop that commit entirely?

ivanperez-keera commented 1 year ago

I dropped the commit in question and rebased everything on top of the current master so that it renders a cleaner merge.

ivanperez-keera commented 1 year ago

@andreasabel just circling back on this.

My expectation for now is to continue sending several of these "cosmetic" PRs, as you called them, until I'm familiar enough with the architecture to suggest bigger changes or new features.

Cleaning the code helps me understand the project better. Even if they are small changes, I hope they also help others in the future to contribute and to maintain the project.

ivanperez-keera commented 1 year ago

Just circling back.

ivanperez-keera commented 1 year ago

It's been 2.5 months, I assume there's just no interest. Closing this.