A bit different approach for refactoring

[mnaoumov]

Fixes #262

A slightly different idea than in #264

[pjkaufman]

This looks good to me. Would main.ts still load values by their name or would that need updating?

[pjkaufman]

@mnaoumov , I am not sure if you are looking for approval on the concept before moving forward or if you just wanted to provide the proof of concept and let a maintainer make the change, so I wanted to reach out and see what the next steps should be for this to make sure we can integrate this kind of change.

Did you want to finish up the refactor or would you like someone like myself to run with this concept and see if we cannot go ahead and make the repo use the new setup for rules as a whole? Thanks for helping me understand where to go from here!

[mnaoumov]

Yes, I'll finish the refactoring during the weekends

[pjkaufman]

Sounds good. I will be off and on available this next weekend, so if I am slow to respond, that may be why.

[mnaoumov]

@pjkaufman I will just ask for a little code freeze during my refactoring project because doing merges might be quite painful and time consuming

[pjkaufman]

Gotcha. I don't plan to make any changes this coming week, and after today, so that should work out quite nicely. I will just add a change or two I am working on right now and then let you have the master branch.

[pjkaufman]

I will be available to make changes again after this coming week, but during the week I will be jam packed with things to do, so I will onky be able to look at things breifly during my free time.

[pjkaufman]

The last change I plan to make is to see about adding a rule for the yaml around deciding what type of syntax to use for a yaml array. I think it should take me about 1 or 2 hours to complete. After that I will create a release and let the master branch sit for you to make your refactoring changes. Hopefully this will not make the refactor difficult.

[mnaoumov]

@pjkaufman I pushed a few more things for approval. It is not fully working yet. But I would like to get rid of the hack that you have with BooleanOption.prototype.display and similar in main.ts

[pjkaufman]

@pjkaufman I pushed a few more things for approval. It is not fully working yet. But I would like to get rid of the hack that you have with BooleanOption.prototype.display and similar in main.ts

I would like that as well. I was not sure exactly how to do it, though I guess I could always ask in the plugin dev chat on the Obsidian Discord and see what ideas people have.

[mnaoumov]

@pjkaufman I am pretty sure I found the way how to do this properly but I didn't have time to actually verify it yet

[pjkaufman]

That would be great if it works.

[pjkaufman]

I am going to go ahead and make a release for what is on master and then stop adding changes while you do the refactor. I realized that the changes I was planning on making for the yaml arrays needs to have a lot more time spent on it than just a couple of hours in order to make sure it works.

[pjkaufman]

The release has been created. I have gone ahead and made the release. Feel free to make your changes to master without additions to the rules in the meantime.

[pjkaufman]

I just wanted to make sure we are on the same page that I can hold making changes on the plugin until this coming Monday, but after that, changes may start being added again. Please let me know if this would cause issues for you.

[pjkaufman]

Is this ready for review? If so, I can try it out locally and see if rules still work as expected and the settings load properly.

[mnaoumov]

@pjkaufman yes, please review. I realized I forgot to update documentation, so I'll do that a bit later.

Please note that the rules order changed. As I switched to the automatically generated list of rules instead of hard-coded, I had to define the predictable order of them. I chose to sort by rule type, then by rule name. Therefore the order of rules slightly changed in the rules.md and README.md

I also noticed that we have some case where we implicitly relied on the order of the rules to be executed. I'll create a corresponding test case to address this shortly

[pjkaufman]

Gotcha. I am taking a look, but one thing that needs chaning is that we need to use window.moment instead of the moment instance we have via the moment dependency unless you have verified that the locale changes are sticking around after changing it in Obsidian itself. The UTs passes, but for some reason I needed to use window.locale for it to work in Obsidian.

[pjkaufman]

I am about halfway through the review and just wanted to make the comments I have available so far.

[pjkaufman]

Out of curiosity did you mean to add your editor config?

[pjkaufman]

Is there a reason to add the editor config?

[pjkaufman]

As a whole, I am not sure how comfortable I am with the generics in the rule builder itself as it abstracts things several layers and is not easy to follow just by reading it. However, I am willing to let it go through assuming it actually does help with property safety during compile time and or when linting.

As for the other issues, I think some tests were missed for removing spaces (they may have been added post the initial creation of the new format of the test last weekend), and the image changes need to be reverted as the gif is no longer acting like a gif and the png does not seem to load.

Other than that, the code looks good. I would just have to test run it and verify that it works as expected.

[pjkaufman]

Looks like we are just missing the following for me to approve the PR:

1. Changing the instances of moment back to window.moment so locale changes work in the Obsidian editor (I am not sure why it has to use the window, but the locales are not loaded otherwise)
2. An explanation of why an editor config was added, so it is better understood why it is present

[pjkaufman]

I am just waiting on an explanation on the editor config addition now.

[mnaoumov]

@pjkaufman I am still trying to figure out how to avoid using global moment. I am pretty sure I found it, just trying to get it working.

Regarding .editorconfig, I already replied above

[mnaoumov]

Without .editorconfig, my editor tends to use 4 spaces for indentation instead of 2, so then linter reformats it essentially. We would like to avoid it. If 2 spaces is the project standard, it's better to help contributors to enforce this standard

[pjkaufman]

@pjkaufman I am still trying to figure out how to avoid using global moment. I am pretty sure I found it, just trying to get it working.

Regarding .editorconfig, I already replied above

Let me know once you have a possible solution to avoid using the global instance of moment and I will gladly try it out locally. Overall this looks great! It will be nice to get it merged in and have the rules be set apart like this. It will help out with several other things as well including adding a warning mode down the road. I look forward to seeing how this turns out in the end.

[mnaoumov]

@pjkaufman I got rid of global moment dependency. The problem was that by default moment is loaded without locales. That's why your moment.locale() calls lead to nothing. I worked it around

[pjkaufman]

Looks good @mnaoumov . Did you have anything else you wanted to do to it before I merge it?

[mnaoumov]

@pjkaufman I don't have more ideas now. You can merge. If I think of something, I'll create a new PR