Fork me on GitHub
#cryogen
<
2020-11-26
>
dorab00:11:25

Hi @U0522TWDA. Github claims that there are conflicts that need to be merged. Would you please investigate and address? I briefly looked over the changes visually and they seem reasonable - given my limited understanding of the cryogen code. Overall, though, I have concerns that by just compiling the changed files, the whole blog is left in an inconsistent state. Furthermore, this is a breaking change in the behavior of the system, which might catch some folks off guard. For example, those that start up a server and push changes to file without re-starting the server. One approach to address my concerns would be to put this change behind a configuration parameter, but with the obvious drawback of the additional complexity of yet another config key. Perhaps also add something to the docs that mentions this changed behavior Being relatively new to cryogen, I'd like input on this from others @U0DJK1VH6 and @U050CBXUZ . I might also be over-thinking this, and my concerns may not be an issue in practice?

Jakub Holý (HolyJak)08:11:39

Thanks a lot! I have fixed the conflicts now. > the whole blog is left in an inconsistent state That is true and it has been discussed before. While it is not perfect, it is a reasonable compromise between convenience (having fast compilation during "development", a feature requested for years) and code complexity. You are not expected to use lein serve to generate your production files. Perhaps we should make that explicit in the readme or somewhere? When you do lein run - presumably on your CI - you are guaranteed to have everything right. Also notice it is opt-in - if you do not send the changeset from your code, everything will be compiled. > this is a breaking change not really, thanks to the opt-in I can try to dig up the previous discussions on this topic with Carmen & Co.

Jakub Holý (HolyJak)08:11:14

Carmen and Dmitri gave their LGTM here https://clojurians.zulipchat.com/#narrow/stream/180378-slack-archive/topic/cryogen/near/215103821, scroll up few posts and back to follow the discussion where Carmen had the same proposal as you about a flag but retracted it eventually after I explained the opt-in .

dorab17:11:42

Thanks to the pointer to the previous discussions. I had not realized it was opt-in. I see that now. Thanks for fixing the conflicts. Github says there are conflicts on the associated PR220 on the cryogen repo. Once you resolve that conflict, I can test out both PRs and merge them. One note - this PR#149 changes the version in project.clj. Perhaps it should be changed to 0.4.1-SNAPSHOT? Or perhaps this PR should not change the version at all, and the version should be changed only when a coordinated set of changes are made?

Jakub Holý (HolyJak)18:11:34

Thanks, conflict fixed

Jakub Holý (HolyJak)19:11:19

Regarding the version: I think it is good practice to change it since this is not part of 0.4.0 and being -SNAPSHOT it is clear it is not made for release. When we decide to make a release, then we change it to whatever we see appropriate and release. But if you prefer, I can drop the version change. Ideally, IMHO, the version would already have been "NEXT" but maven does not support that 🙂

dorab20:11:01

Okay. Thanks. I have to step out right now, but will review and merge later. The version issue is another thing that could be discussed in #cryogen .

dorab02:11:46

@U0522TWDA I reviewed and merged cryogen-core PR#152 and the corresponding cryogen-docs PR#39

dorab02:11:10

I also noticed that the PR bumped up the version.

Jakub Holý (HolyJak)08:11:10

Right. Perhaps we should write down guidelines for when, how to bump the version?

Jakub Holý (HolyJak)08:11:39

Thanks a lot! I have fixed the conflicts now. > the whole blog is left in an inconsistent state That is true and it has been discussed before. While it is not perfect, it is a reasonable compromise between convenience (having fast compilation during "development", a feature requested for years) and code complexity. You are not expected to use lein serve to generate your production files. Perhaps we should make that explicit in the readme or somewhere? When you do lein run - presumably on your CI - you are guaranteed to have everything right. Also notice it is opt-in - if you do not send the changeset from your code, everything will be compiled. > this is a breaking change not really, thanks to the opt-in I can try to dig up the previous discussions on this topic with Carmen & Co.