After completing the bare-bones Javascript export option feature that exports only a boilerplate project, get it merged into the main ESCET repository. Align with ESCET developers and document the process for the future.
Designs
Child items
...
Linked items
0
Link issues together to show that they're related or that one is blocking others.
Learn more.
I'd rather not do this as a last step, but do this for each of the other steps of this milestone. Then you get early feedback and experience with the process, and any feedback can already be taken into account for the next step. Also, it prevents a merge request to ESCET with several thousands lines of code, templates, etc, etc, that will take a lot of time to review. If you can develop step by step, and also contribute step by step, with developing always a bit ahead of the contributing it back, I think that would be ideal for all involved.
@ddennis I'd like to create discrete branches/commits on this repo for each ticket, and ask for micro-level reviews or help there, if that's okay. That would take care of the micro-level discussions and details on this repo, so that for a merge back to the main repo (towards the wider community and for the history logs) we should only have to handle the changes at the macro-level (a condensed summary of changes and a minimal testable feature/increment, coupled to a ticket on the main repo). I do of course constantly pull in changes from the main repo to keep up to date.
I've noticed on earlier projects that with CI/CD, the overhead cost of synchronisation (everyone constantly keeping up to date with even the smallest changes) tends to be ignored, as if there is no cost, in favor of a false sense of security (people superficially reviewing small changes for typo's etc, but not looking holistically at a feature/contribution or being able to run/test/verify anything). I think it's inevitable that reviewing a merge is going to take time, but combining all changes together into a minimal package that constitutes a deliverable, testable increment is a way to maximise the quality of testing/reviews, and makes best use of people's time. This does require that work can be cleanly compartmentalised. The most delays I've had working on projects is the overhead of having to wait for each other's approval (locking/blocking calls in asynchronous programming, basically), as well as people having to deal with each other's personal details while we're all constantly task switching and paying minimal attention to the bigger picture. In short, I think there's a happy medium, where the local does the local and the global does the global, and the need for synchronisation (and accompanying overhead) is balanced against the risk of decoherence (larger when people are working on the same code, smaller when working within distinct compartments). A bit like raising a child and teaching it manners before exposing it to society, or creating an idea or product in a messy workspace before presenting it cleanly. A lot of the mess should be handled inside the family, though we do of course need to properly socialise our children ;).
In this case, I thought adding a button to the export menu and having it generate boilerplate (empty) export packages (for "source code" and "simulator" targets) would be a convenient deliverable/testable increment, with all relevant code/classes combined instead of committed separately/incrementally(?).
This is somewhat of a philosophical argument, if you'd like me to just merge every commit back to master a.s.a.p., you can say so. I'm trying to find the optimal way for handling these situations, as they keep occurring in different contexts (both code and real life), and I love thinking about these things.
It is fine to mature the code in your clone first, before contributing it. I can indeed do a pre-review, if you want. Will you create merge requests within your clone for that?
Generally, very large contributions take a long time to review. And thus it requires having a significantly large block of time to do the review (to prevent interruptions that cause inefficiency). So, it may take hours or days before the review can even start. If the contributions are smaller, finding some time in between other work is much easier. So, if smaller, then the changes of you being delayed are smaller.
What is then a good 'size' of a contribution? I agree 'every commit' is too granular, although that also depends on the size of the commits ;) I assume most issues will lead to multiple commits, such as adding the new target language, using it in the code generator, adding some documentation, updating/adding tests, etc.
Adding two new targets, and having them generate empty files seems reasonable indeed. In my opinion, that should include a very minimal addition to the documentation, such that users can read that the new target exists, but should not be used yet (see some comments elsewhere).
I still think it is best to contribute back after each issue in this repo. This keeps the merge requests smaller. Per milestone seems to big, as then we get everything in only 3 merge requests or so. If possible, I'd like to ensure the merge requests are not too big, say at the very most a 1000 lines. But this is not a hard limit. For instance, if you add the expected test output for existing test models, and this is just generated output, that wouldn't be counted. Note that large contributions also require a review by the Eclipse Foundation IP team, which is time consuming, and thus another reason to avoid large contributions.
The difficult part is likely that you'll be continuing on, while the review is taking place. If that results in changes, you'll have to integrate these in the follow-up branch again. Depending on the nature of the changes, this may be simple or take considerable time, is my experience. I struggle with this as well sometimes. It seems unavoidable.
I don't think others will be working on the code generator, its tests, or its documentation at the same time as that you are working on it, so the potential for conflicts is likely low. If there are any, they'll likely be easy to resolve.
Thanks for the feedback, I'll make separate branches and commits on this repo to keep reviews manageable. If changes need to be made before merging to the main repo, and I'm already working on the next thing, then I can at least align things on my end with minimal effort and without bothering anyone else. When we merge back to the main repo, ideally we can squash any irrelevant in-between steps and only review/test the final changes (which should already have been approved/tested at the micro level while I/we were working in this repo).
We'll use this first milestone as a bit of a pilot to work out the process, hopefully it'll go smoothly, otherwise of course we discuss and course-correct.
You can certainly squash/fixup commits after processing comments. But, don't do that while someone else is still reviewing it. Also, fully squashing everything before contributing back may be too much. If the changes are large, it may be better to have multiple commits, especially if multiple changes happen. Otherwise the single commit may be hard to review.
*All branches basically have 1 commit, under 1000 changes.
Has been reviewed by Gunnar and Anouska. The only comments they had were that some of the CSS formatting is inconsistent (using shorthand or not for colors #FFF or #FFFFFF, etc), and I could swap the order of export options/texts to be consistent everywhere (so, basically only style things). I'd like your comments before continuing. Is there any standardised css/html/js style for ESCET? Let me know if you'd like anything changed.
I'll look at adding documentation for the new JS export option. I can make merge requests to my own dev branch and the main repo as you like, and of course rebase (pull in changes from the main repo) before merging back. Right now it's only the few commits with my changes, so easy to review I hope. Let me know how you'd prefer to continue, thanks!
I've reviewed branch/commit 1. My comments (it would be easier as a merge request, because then I can add it to the line itself, rather than a list here):
general
Javascript -> JavaScript (that is the official name), in help texts, comments, class names, etc. And then also javascriptSomeVar -> javaScriptSomeVar, etc.
I'm missing the minimal documentation update that I asked for. I'd like it to be present in the first merge request.
CodeGenApp
In CodeGen, you add JAVASCRIPT after JAVA. Here, at the end. Keep the order the same everywhere. I'd put it everywhere just after JAVA.
Use auto reformat to format all files. Ctrl+Shift+F.
I assume you'll be adding something to the JavaScript option sub-category later on?
JavaScriptCodeGen
I'd add // TODO To be implemented' after the lines in the methods that are essentially placeholders (like return null;etc). Also inJavaScriptTypeCodeGen`.
new HashSet<>(); -> We always use the set method from our Sets class. It ensures we get ordered sets, and thus deterministic output. Just type set, and the quickfix can add the static import.
JavaScriptTypeCodeGen
CIF type to convert to the Java language -> CIF type to convert to the JavaScript language
main.txt
Shouldn't the title be part of the head?
It may be nice to align the HTML output to the website, see releng\org.eclipse.escet.releng.website\website\index.html. So, include the charset, see if you can use similar styling, etc.
I'd name this index.txt, as the main file being generated should probably be index.html, as that is customary, right?
I'm pretty sure the empty method bodies give warnings. I don't want any warnings in develop. Please add a // TODO To be implemented. comment in each method. And auto reformat.
TargetLanguageOption
Split the string, to ensure the 120 character line limit is honored.
Thanks! I meant to create merge requests to my develop branch and/or back to the main repo for reviews, rather than do it here. Was wondering how/where you'd like to proceed in that sense, and if there were any fundamental things that still need doing/changing. I'll make the changes discussed here of course. For the others, I can make merge requests as you like, depending on how/where you'd like to review them (this repo or main).
I still need to find where I should add the documentation, by the way(?).
You could make a branch in your repo for 'done' things. Then each merge request would be a branch that goes from there. Branches that build on top of that, could be merge requests that go to the branches of other merge requests. Once a parent merge request gets merged, the target of the child is automatically updated.
Regarding documentation:
Main page of the CIF code generator documentation: cif\org.eclipse.escet.cif.documentation\asciidoc\tools\codegen\index.asciidoc. It needs a few additions for the new target language at lest.
You can find the other files in the same directory. I'd copy the java.asciidoc file to javascript.asciidoc. I'd remove all the content and instead add a warning that it is work in progress. See for instance cif\org.eclipse.escet.cif.documentation\asciidoc\language-tutorial\extensions\annotations.asciidoc for how to add a warning.
If you add a new file/page, you need to add it also to cif\org.eclipse.escet.cif.documentation\asciidoc\documentation.asciidoc.
Should take into account all feedback, except "I assume you'll be adding something to the JavaScript option sub-category later on?" - Yes, we'll see what we need (single file export vs export package of files is one).
@ddennis As an example (and working ahead a bit), I've generated an export for the bouncing ball example, and have manually added the svg element and some JS code to make it work: bouncing_ball_export_v1.zip
The code still needs work (so the ball doesn't go through the floor, etc), but take a look and let me know what you think about the pages so far. Thanks!
I had a quick look at the bounding ball example. Some comments:
I like the look and feel.
When I resize the log panel, the mouse moves much further than the panel. I need to go beyond the screen edge to another screen to really resize it to be very small.
When I resize the log panel, to move it upwards, it first drops a bit downwards, before moving upwards.
I'm not sure I like the yellow background. Maybe I'd only use it as background for the title and the start/stop etc button areas, and not for the area with the SVG image. Or only for the title.
The area below the text entering box seems high. Could reduce it to save some space.
The log output is not in the same syntax as the CIF simulator. The simulator uses 'var=value', not 'var: value, and the simulator separates multiple variables by commas, for instance.
To fit the minimal styling, I was wondering if instead of the Show/Hide texts, it may be nicer to use Unicode symbols 'U+25B2 BLACK UP-POINTING TRIANGLE' and 'U+25BC BLACK DOWN-POINTING TRIANGLE'. See https://symbl.cc/en/25B2/ and https://symbl.cc/en/25BC/.
I've tried to fix this as best as I could, unfortunately this is due to the resize css attribute being a bit quirky. A different implementation would be more complicated, so I've kept it as simple as possible for now (fancy html/js later).
"The log output is not in the same syntax as the CIF simulator. The simulator uses 'var=value', not 'var: value, and the simulator separates multiple variables by commas, for instance."
The log output is something we'll need to align (and still needs to be properly implemented), will also need to look at a fmt() function in JavaScript that functions the same way CIF/Java's fmt/format does. I've kept the fmt() function in JavaScript simple for starters.
"Colors/icons"
We can make things look pretty later? We can go back and forth a lot on cosmetic details, might be easier to sit together for that, or try a few things yourself as an example?
I reviewed the 2nd branch, where the utils are added. I only did a quick review, but this should give you more than enough for now to process:
general
"Did not add more changes to keep the commit under 1000 lines." -> Please don't mention this in the commit message or merge request. We don't want to seem to be circumventing the rules.
The template has trailing whitespace, that should be removed.
I've noted a few things below for the utility methods. Some of those comments also apply to other utility methods.
absInt: This supports a larger range of numbers than CIF. According to https://www.w3schools.com/js/js_number_methods.asp: "Safe integers are all integers from -(2^53 - 1) to +(2^53 - 1)."
Hence, the 'abs' function in JavaScript doesn't need to handle special values for its range, as the positive and negative range limits are the same.
However, see https://gitlab.eclipse.org/eclipse/escet/escet/-/blob/develop/cif/org.eclipse.escet.cif.metamodel/docs/cif_ecore_doc.pdf for the CIF metamodel documentation.
It states: "The value of integer value literal expressions is encoded using the EInt datatype, which matches 32-bit integers in the range [−2^31 .. 2^31 − 1]."
We should limit the range to that, I think. I'd define a MIN_INT and MAX_INT to be the same as Java's int and use that instead of Number.MIN_SAFE_INTEGER and Number.MAX_SAFE_INTEGER.
equalObjs: It seems JavaScript docstrings support backticks. So, rather than {@code null} use null between backticks.
escape: Double space after -.
escape: See https://stackoverflow.com/a/784547 (first comment), which indicates: "Just additional note: str.replace("\n", '...') (first argument is a regular string) will replace only first occurrence"
Use a regex instead, as indicated by that solution?
exp: I Googled whether HTML is supported in JavaScript docstrings, but couldn't find anything about it. Are you sure it works?
fmt: How did you come to this implementation of the fmt function?
intToReal: According to https://www.w3schools.com/js/js_numbers.asp, JavaScript always uses 64-bit double values internally. I don't think there is a 'JavaScript Integer (number) value`. Don't use Java class names that don't exist in JavaScript.
makelist: Is this really needed? Can't we use [..., ..., ...] instead? In Java, we need the method due to oddities related to primitive values vs objects, if I remember correctly. I don't think that applies to JavaScript.
mod: Semantics seems to be the same, even for negative numbers. However, -8 % 4 and -8 % 4 return -0, which we don't use in CIF. We use only 0, aka +0. See e.g. multiplyDouble.
realToStr: This doesn't work. For instance, (1.0).toString() returns 1, which is not CIF syntax for a real literal.
Syntax for a CIF real literal: "(0|[1-9][0-9]*)(.[0-9]+|(.[0-9]+)?[eE][-+]?[0-9]+)" (see cif.setext).
project: I see multiple project functions. Why don't we need projectList, projectString etc, like absInt and absDouble?
strToInt: This probably accepts too large/small numeric values.
There is inconsistent indentation (2 vs 4 spaces) in the file.
There is inconsistent capitalization of colors. I propose to use lower case letters in colors.
main.txt
Why is some styling inline and the rest in a separate file? Seems inconsistent.
The empty lines after <style> and before </style> can be removed.
I don't know whether all the CSS is needed, correct, etc. I didn't test it. I only reviewed the changes in GitLab.
I propose to use lower case letters in colors.
height: 300px; -> Is a fixed size a good idea. Wouldn't a percentage be better? There are very different screen resolutions in today's world.
margin: 0px; -> For margins, padding, etc, if its is zero, the unit may be omitted. So margin: 0; is nicer in that case, I think. May apply to other attributes as well.
span{ -> span {.
elem.value += message + '\r\n'; -> Can you explain the use of \r\n. For HTML, I'd have expected <br>?
function showHide -> I'm not sure about this design. I'd have expected two classes, expanded and collapsed for the log pane. Then here, you add one and remove the other. Then all the styling you set here can just be in the CSS. If something can be done in CSS, I'd prefer that over using JS.
if(...) { -> if (...) { in various places.
if(!elem.value.includes("=") && !elem.value.includes(")")) { -> I get the 2nd part, but why check for an equal sign?
The idea is to use this to test the generated JavaScript, and compare results with hardcoded values so we can confirm we get the same results across platforms. Could be useful for developers/debugging only, in which case we could remove it?
"Why some styling inline and the rest in a separate file"
The separate file is the common ESCET css for all exports, the inline css is specific to the kind of cif file being exported (different features might use different UI?). If it turns out that all cif exports will use the same HTML/CSS in the end, we can merge these.
"Can you explain the use of \r\n. For HTML, I'd have expected <br>?"
The textContent of the input isn't parsed as HTML.
"if(!elem.value.includes("=") && !elem.value.includes(")")) { -> I get the 2nd part, but why check for an equal sign?"
To exclude any JavaScript console commands that aren't a function call, like variable assignments. Ideally, users can type a function name in the console without "()", and it'll run. It should of course only do this for console commands that are function names.
@riklubking I see you're starting on tasks from milestone 4, and still no merge request to ESCET at all. I think you need to prioritize more that stuff gets merged back, as discussed during the last sprint meeting.
Hey @ddennis, I'm committing and stashing what I have, and including your feedback so that branches can be merged. Since some files were renamed, I couldn't easily rebase, so I've re-done some branches and commits as cleanly as I could.
I'm finishing up the first 3 branches for merge, will notify you asap.
Working on the fourth at the moment. I need to see about (re-)naming commits with issue names, I assume this will require creating an issue for each merge on the main repo?
You can use the same issue number for all merge requests, if you want. Just make separate merge requests for the different parts. I'll see if I can review the latest changes in the weekend.
For the 3 branches, I'm creating merge requests, so you can comment on those instead of here, if you like. I've only applied your feedback (where possible). This thread was originally intended for high-level feedback to prepare merges (until we had a process), so it's gone beyond scope. Cleaning things up at the moment.
I'm finishing the 4th branch at the moment which generates the variables and events/functions. The problem I have is testing whether exported code works properly, for which ideally I'd have working SVG/UI. I can create this manually, but I'd like to generate the SVG element(s) in the HTML. What is the proper way of reading SVG tags from a CIF file? I've looked for the SVG information on the CifSpecification object used by the CodeGen code, used when reading the other tags and generating code, but the SVG tags appear to be filtered out (don't exist on the CifSpecification object). Which existing code should I use to read SVG tags from CIF files? Thanks!
For the 3 branches, I'm creating merge requests, so you can comment on those instead of here, if you like.
That would be great. Then I can see the new commit separately, but also the full diff.
I've looked for the SVG information on the CifSpecification object [...]
That class does not exist. Do you mean Specification?
[...] the SVG tags appear to be filtered out (don't exist on the CifSpecification object).
Indeed, the CIF code generator currently removes the CIF/SVG declarations before code generation.
You'll have to make that dependent on the target language, I suppose.
What is the proper way of reading SVG tags from a CIF file?
You can do it in the same way as the print declarations are currently collected.
Currently, the only tool that uses the CIF/SVG declarations is the CIF simulator, which generates code for them for simulation purposes.
I can't make merge requests depend on each other due to rights on the main repo (I can on my own repo). I've prepared the merge requests here for now (targeting my own develop branch):
Gitlab allows me to make one merge depend on another, but unfortunately only allows this to chain once, so merge 2 requires merge 1, but merge 3 can't then also require merge 2. Also, commits included in a previous merge are still shown (until the previous merge is done). A little messy, different than what I'm used to when using Git. The idea was of course that all commits be listed in proper order and each merge has only the commits relevant.
Should be easy though, only 2/3 commits per branch, I've added all information. Take a look and let me know how to continue, if I could link/chain the 3 merges I would have targeted them all to develop on the main repo and offered all 3 merges at once, now it looks like I may have to offer the merges one by one to the main repo.
I'll also respond to the rest of your feedback in a minute, any questions and anything I couldn't include.
Gitlab allows me to make one merge depend on another, but unfortunately only allows this to chain once, so merge 2 requires merge 1, but merge 3 can't then also require merge 2.
That works for us as committers, as we have all the branches in the official repo, and can target the merge request to merge to the branch of the other merge request. Maybe because you as contributor are using a fork, that is not possible.
[...] it looks like I may have to offer the merges one by one to the main repo.
Let's do that then. Can you already make the first one?
I've prepared the merge requests here for now (targeting my own develop branch)
I don't think you want to target develop. Then you can't pull in changes from the official ESCET repo anymore. I'd create a separate integration branch in your repo then, and merge them in there. But, why not create the first one as a merge request on ESCET itself? There is no dependency for that one. Then we'll just do them one by one, as you indicated already.
Will do them one by one. First merge to the main repo incoming. I may have to fix a Jenkins build error first, let's see.
I've got a fourth branch for code generation (vars and functions etc), have been looking at SVG/simulator features so I can test code/exports before committing. I'm trying to create a modified version of CifSVGGenerator that works within the context of CodeGen, but that's quite complicated. I've asked Jeroen to move the retro meeting to next week, since I think that will be enough time to get SVG working, and then we can actually show something rather than just code. For now, I'll just test by mocking simulator code manually, and will commit the fourth branch asap.