Refactoring

[Jan-Ka]

This is a WIP Pull Request - should make it easier to talk about what looks good etc.

I saw that @waldyrious started #36 , something I was actually already preparing to show. Everything up to https://github.com/waldyrious/primerpedia/commit/507f7ffb8ac7be3e4f176bdc2dd3d24798bf259f is just preparation work.

Things to do:

• apiRequest is using things from the main file, to keep it DRY we need to refactor it a tad further
  • first step was to move the spinner rendering out of the function
  • maybe overkill but it might be a good idea to add another file that handles configuration (maybe interesting for #9 as well)
  • then we could move apiRequest to utils.js and access the common settings file even though that would still mean that apiRequest had some state/temporal coupling going on it already does since it has to use global scope.
• All of the URLS are only cobbled together - it works but there has to be a more elegant solution
  • I extracted and grouped together most of the query parameters at the top to see if there's a pattern but sadly everything is only similar but not equal

[waldyrious]

I saw that @waldyrious started #36 , something I was actually already preparing to show.

Sorry to spoil :sweat_smile:

Would you like me to review now, or do I wait until you're done? I'm fine with either approach.

[Jan-Ka]

Haha, sorry - I didn't mean that in a confronting way. :D

If you like it would be cool if you could take a glance and tell me if I worked in the right direction.

[waldyrious]

I'd suggest putting this on hold until the other PRs (including #32) are merged, to avoid merge conflicts and making the git graph too messy.

[waldyrious]

@Jan-Ka #32 is now out of the way :tada: -- it's probably best to wrap up this one as well to avoid accumulating even more conflicts. I'd suggest either doing it before #54, or doing it immediately after it. Let me know if there's anything here I can help with.

[Jan-Ka]

:tada: indeed :D

Maybe I can verify everything today, anyway I will do it asap. Don't think I need your help since this is rather straight forward really.

[Jan-Ka]

To be honest, rebasing this was no fun at all. But it is now done and I hope in an agreeable manner.

@waldyrious You might want to check the commits after 3b4d58d364733b058c597808302e755f6d9d2e86. Not sure how to feel about the configuration comments for eslint but they are an necessary evil.

[waldyrious]

To be honest, rebasing this was no fun at all.

It takes a special kind of masochism to enjoy git rebasing, truth be told :smile:

I'll take a look at the commits.

[waldyrious]

I've added some comments inline (for some reason github shows them as applying to outdated diffs -- any clue what might be the issue?).

Anyway, here are a couple extra comments:

• Commit messages that say "move ... in ..." should be instead "move ... into ..."
• I don't think it's worth adding a separate commit adding an empty file (a6e5d08), nor loading it before it has any content (bef9e34)

Let me know what you think about the other comments. Once we're in agreement I'll do the rebase / edits myself, since both the conflicts and the requested changes are my responsibility.

[Jan-Ka]

Thanks for the tips.

I went through all comments, nothing to complain about.

As for my reasoning for the empty file. I did not want to put the addition of the file and the moving of the utilities into the same commit. I actually wanted to put the call from the HTML and the addition of the file in the same commit but somehow screwed that up.

[waldyrious]

But why didn't you want to do so in the first place? I don't see the benefit, top be honest.

[Jan-Ka]

It felt like a logical separation. Add a File, move Stuff into the File.

[waldyrious]

Hm... I still think it isn't warranted. I mean, by the same logic, we could use one commit to create empty function shells (signatures with empty bodies) in this new file, then move the contents in another commit, and delete the contents from the original place in a third commit.

My point is, we can always subdivide changes into smaller pieces, or group them into larger chunks (e.g. squash an entire feature branch down to a single commit), but there's a level where grouping them makes semantic sense, i.e. they correspond to what a human would consider a single action: a self-contained, logical, atomic change.

In my view, moving functions from one existing file to a new file counts as a single logical action. But hey, if this still doesn't convince you, no problem -- there's no harm in adding an extra commit :smile:

[Jan-Ka]

Sorry if I made it sound like I'm defending my decision in this case, I just tried to explain and how and why. I do not have any objections against combining this commits.

I am not really used to this level of commit grooming - I guess I have to invest some time to understand working with Git better.

Separating the file and content changes probably is more natural to me since I work mostly with .NET where adding a file also changes the project file which can lead to problems if you have additional changes rely on that. :)

[waldyrious]

I am not really used to this level of commit grooming - I guess I have to invest some time to understand working with Git better.

Nah, don't worry about that :) git can get as complex as one's willing to go. For me it's more a matter of personal preference for cleaner repository graphs, mixed with the challenge of mastering the tool a bit more every time I use it. In reality, none of this is indispensable and I certainly wouldn't want others to feel compelled to spend more effort on grooming commits than they'd be naturally inclined to -- these small things do eat up energy over time, which is much more valuable to the project in other ways ;)

Separating the file and content changes probably is more natural to me since I work mostly with .NET where adding a file also changes the project file which can lead to problems if you have additional changes rely on that. :)

Ah, that explains it!

So, regarding this PR, are we in agreement in all the points raised above? Shall I make the changes? Let me know if there's anything else you'd like to clarify / discuss.

[Jan-Ka]

Yes, we agreed on all issues. Please make the changes. :)

[waldyrious]

@Jan-Ka it took me a couple extra passes to get the rebasing right (I kept forgetting details we had discussed above). Please make a final review and let me know if everything looks good to merge.

By the way, please review these seven rules of a great git commit message, in particular rules 1 and 5. The first one is especially important, because Git expects the blank line between the title and the body of the commit message, otherwise the second line is treated as part of the title and gets shown as a single long line in the CLI and other interfaces.

[Jan-Ka]

Looks good to me. Thanks for the link.