Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two-space indentation #1847

Closed
edwardalee opened this issue Jun 15, 2023 · 7 comments · Fixed by #1851
Closed

Two-space indentation #1847

edwardalee opened this issue Jun 15, 2023 · 7 comments · Fixed by #1851

Comments

@edwardalee
Copy link
Collaborator

This is a bike-shedding issue that we should should either act on now, while we are in the middle of reformatting pain, or close permanently. I'm fine either way.

The issue is that two-space indentation does not look very good with a two-character delimiters for target code. For example, from our test suite:

  reaction(startup, a) -> a {=
    if (!a->is_present) {
        if (self->offset == 0) {
            printf("Hello World!\n");
            self->success = true;
        } else {
            lf_schedule(a, self->offset);
        }
    } else {
        printf("Hello World 2!\n");
        self->success = true;
    }
  =}

The last two lines are what I'm concerned about. This does not look good to me. A separate issue is that the code body indentation is left alone (as it probably should be), and in this case, it uses four spaces. With four-space indentation all around, it looks like this:

  reaction(startup, a) -> a {=
      if (!a->is_present) {
          if (self->offset == 0) {
              printf("Hello World!\n");
              self->success = true;
          } else {
              lf_schedule(a, self->offset);
          }
      } else {
          printf("Hello World 2!\n");
          self->success = true;
      }
  =}

This looks better to me. I do not feel strongly about this and am fine with closing this issue if we want to stick to two-space indentation.

@cmnrd
Copy link
Collaborator

cmnrd commented Jun 15, 2023

Just for completeness, I would like to add the example with 2 space indentation also in the target code.

  reaction(startup, a) -> a {=
    if (!a->is_present) {
      if (self->offset == 0) {
        printf("Hello World!\n");
        self->success = true;
      } else {
        lf_schedule(a, self->offset);
      }
    } else {
      printf("Hello World 2!\n");
      self->success = true;
    }
  =}

I can relate to the concern about the last two lines. To me personally, however, it still looks fine with the 2 spaces. Maybe this is because I am used to 2 space indentation from other projects. So my vote would be for closing this issue.

I am undecided about what we should do about the target code. We could try to hook in with the target language formatters, but I am very skeptical about this path. The root of the problem is that we are mixing two languages, and perhaps we shouldn't do this in the first place.

@lhstrh
Copy link
Member

lhstrh commented Jun 17, 2023

I'm not sure that changing from 2 to 4 spaces fixes anything.

If you are expecting the closing brace to be one indentation unit of indentation removed from the inner the code block, then that also won't be the case with 4-space indentation: the } will be 3, not 4 spaces removed from the inner } (due to the preceding =).

I agree that it may be easier to spot the difference between 1 and 2 spaces than between 3 and 4, but that doesn't change the fact that the indentation will always be off by one because of the extra character that we use...

@edwardalee
Copy link
Collaborator Author

OK, I give up. Please adjust all the tests and examples so that they use two-space indents in the reaction bodies. Currently, with the mix, they look pretty ugly.

@edwardalee
Copy link
Collaborator Author

One request is still not done: setup instructions for IntelliJ and VS Code need to explain how to set default spacing to two, which no IDE that I've encountered defaults to.

@cmnrd
Copy link
Collaborator

cmnrd commented Jun 21, 2023

This was autoclosed by the merge. I think we should try to make 2 spaces the default where possible, instead of relying on the user to make this change. For the developer IntelliJ we might be able to control this either in the version controlled configuration files or in the gradle setup. For Epoch and the VS Code extension, it should be possible to create artifacts that use a 2 space default.

@cmnrd cmnrd reopened this Jun 21, 2023
@lhstrh
Copy link
Member

lhstrh commented Jun 21, 2023

In VSCode, the default setting is Detect Indentation, which I have found to work fine. Perhaps @a-sr knows how to instruct Epoch to use two-space indentation.

@a-sr
Copy link
Collaborator

a-sr commented Jul 6, 2023

Perhaps @a-sr knows how to instruct Epoch to use two-space indentation.

lf-lang/epoch#21 will configure Epoch to use 2 spaces for tabs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants