Refactoring suggestion

[kachkaev]

Hi again @shd101wyy!

I'm currently playing with {cmd=true} and as a by-product of that would like to offer some help with code refactoring. I could start with splitting markdown-engine into a set of smaller files, which will make the code much easier to read and decouple. For example, I could help with moving export functions into a new folder called src/export rather than keeping them all in one large class. Similarly, custom syntax rules like math, emoji, critic markup etc. can go into individual files and thus become easier to update or cover with tests. Ultimately, some functionality that has been extracted into smaller and cleaner files can become node modules on their own, which sometimes happens with other libraries.

Before I start refactoring bits of mume I'd like to ask if you're happy with this suggestion in general. My guess is that the files are so large at the moment because mume started small but then grew beyond its expectations, but I might be wrong and the large files are your design decision. In the latter case extracting smaller modules might not be something you're looking forward to see.

Curious to know what you think and am happy to help if you consider refactoring potentially useful! Mume is awesome!

[shd101wyy]

Yes that would be super nice!

…

On Jan 17, 2018 12:32 PM, "Alexander Kachkaev" ***@***.***> wrote: Hi again @shd101wyy <https://github.com/shd101wyy>! I'm currently playing with {cmd=true} and as a by-product of that would like to offer some help with code refactoring. I could start with splitting markdown-engine into a set of smaller files, which will make the code much easier to read and decouple. For example, I could help with moving export functions into a new folder called src/export rather than keeping them all in one large class. Similarly, custom syntax rules like math, emoji, critic markup etc. can go into individual files and thus become easier to update or cover with tests. Ultimately, some functionality that has been extracted into smaller and cleaner files can become node modules on their own, which sometimes happens with other libraries. Before I start refactoring bits of mume I'd like to ask if you're happy with this suggestion in general. My guess is that the files are so large at the moment because mume started small but then grew beyond its expectations, but I might be wrong and the large files are your design decision. In the latter case extracting smaller modules might not be something you're looking forward to see. Curious to know what you think and am happy to help if you consider refactoring potentially useful! Mume is awesome! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB0gf8iBP9e8kIq4Mb5lw_f61bBtrFA1ks5tLjzSgaJpZM4Rhyet> .

[sheljohn]

I would like to get involved in this project if possible/needed; would you be able to write a short guide for contributors (general code structure, short-term and long-term to-dos, etc) in the wiki?

[kachkaev]

Great to hear that you'd like to get involved @sheljohn! If you're interested in writing a contribution guide, how about making it a part of the repo? What would be also awesome is to have a folder with some test markdowns that use different features of mume. When I'm changing something, It's quite hard to check if you haven't broken anything 😅

[sheljohn]

@kachkaev Markdown tests would be good, but I'm not sure where to start with regards to the contribution guide; any chance either of you could write a high-level version to get started?

[kachkaev]

#40 should partially address refactoring. Here's what our further steps could be:

• Cleaning the code base using tslint and prettier. As a part of Split markdown engine: render enhancers #40, I added two scripts to mume's package.json: npm run lint and npm run format. Linting consists of three parts: TypeScript compilation, tslint and finally, prettier-check (saw something similar in apollo-server).

  Although I could change the code so that linting passes in Split markdown engine: render enhancers #40, I deliberately decided not to go for it because otherwise the PR diff would be enormous and thus hard to review. @shd101wyy if you have a bit of time and are interested in getting all the code look like an masterpiece, it'd be great if you could call npm run format yourself, then kick npm run lint until it passes and finally commit the result on top of Split markdown engine: render enhancers #40 or as a new PR once Split markdown engine: render enhancers #40 is merged. To achieve an even a higher code quality and to make future life easier, you also can toggle "noUnusedLocals" to true in tsconfig.json and thus ensure that we never leave unused imports and local variables while making changes. Enabling this option will cause a few extra errors and might require some of your knowledge on the existing code.

  Once linting has passed, we can instruct all future contributors to enable Prettier on file save in their editor. The good thing about tslint + prettier-check + prettier compared to a very strict tslintrc / eslintrc is that contributors are never annoyed by too many alarms caused by an extra space somewhere, yet get a 100% coherent code style at all times. As a bonus for finishing the linting work, we'll be able to set up TravisCI to automatically do npm run lint and npm run test for every new commit! 🎉

• Switching to promisifed fs functions by fs-extra. I noticed that some of the helpers in src/utility.ts are just promisifed fs functions. We can reduce the size of mume if we delegate this quite basic functionality to an industry-standard module. Given that native fs promises are coming to Node 10 and fs-extra maintainers are aware of that, we can get even more benefits from this change in future. I've already added fs-extra as a dependency and used it in src/render-enhancers/fenced-code-chunk.ts to give you an example of what I mean here (see Split markdown engine: render enhancers #40).

• Reconsidering what a code chunk is (i.e. what is a diagram or any other literate code block and what is arbitrary user code that is controlled by enableScriptExecution flag). Some thoughts are in Split markdown engine: render enhancers #40 (comment).

• Further splitting markdown-engine.ts into smaller modules. This file is 2085 lines of code after Split markdown engine: render enhancers #40 compared to 2934 before my first PR in December, which is about about 30% less. However, there's still plenty of code that can be offloaded to smaller modules and the first obvious candidates are the export methods. To be honest, I'm not sure I'll be able to manage splitting these are there are some bits that I don't understand, but I'd be really happy to see this change in mume at some point! Ideally, markdown-engine.ts should just remain a narrow glue-object that keeps the state and works as an API for Atom, VSCdoe or a command-line client. Once this is achieved, the entire mume can be covered with tests at all levels! ✅

[kachkaev]

@sheljohn

any chance either of you could write a high-level version to get started?

I might do so once #40 is sorted. Meanwhile, gathering a bunch of markdown files that cover the entire mume's functionality would be really awesome! If you're interested in helping with these, you can take a look at test/integration/fixtures folder in #40 and supplement that set of files with anything you want so that every bit of what markdown-preview-enhanced can do is covered. I don't mean writing another bundle of docs, but rather a comprehensive test playground for future contributors. WDYT?

[sheljohn]

@kachkaev I just had a look at the folder, this looks great. Any recommendation on how to test for various header options / exports without creating tons of files? I will have a look at that this week-end either way :)

[kachkaev]

Not sure if there is a way to have few fixture files when we want to test a lot of functionality. Well, we could try stuff everything mume can do into a couple of very long markdowns, but that would create a testing nightmare IMO. As long as the files are carefully organised into subfolders and meaningfully names, we should be good 👍

[kachkaev]

@shd101wyy would you be happy to go through task 1 in #33 (comment) this weekend? If not, I can help you with that. Really looking forward to have stricter tslint rules and to be able to apply prettier everywhere! 🕺

I'd do all that hair-combing a while ago, but I'm a bit confused about:

• producing a huge diff that you won't be able to go through
• removing unused variables in a few of places, which can be actually needed, but are marked as unused due to typos

It'd be really good if you could take that task. Of course, if you have time and desire 😉

[shd101wyy]

Sure I will give a try. Thank you :)

…

On Thu, Mar 22, 2018, 6:19 AM Alexander Kachkaev ***@***.***> wrote: @shd101wyy <https://github.com/shd101wyy> would you be happy to go through task 1 in #33 (comment) <#33 (comment)> this weekend? If not, I can help you with that. Really looking forward to have stricter tslint rules and to be able to apply prettier everywhere! 🕺 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB0gfz3xBY42796qlgl4OW5Fz6pEOUfGks5tg4iogaJpZM4Rhyet> .

[kachkaev]

Yay! I can help you with setting up CI later on. Feel free to ping me tomorrow or this weekend if you have any issues!