Prettier Setup

[jiji14]

Related Issue

For considerations regarding Prettier, please refer to the following GitHub issue: Prettier Issue #1014.

Configuration Options

Our team has carefully chosen these configuration options, which are based on discussions.
To explore more details, please visit the Prettier documentation on options.

How to Run Prettier

You can format your code using Prettier by running the following command:

npx prettier --write {filename}

Auto-Saving

If you wish to enable auto-saving locally, please follow these steps:

1. Create a .vscode folder at the project's root directory.
2. Inside the .vscode folder, create a settings.json file and include the following code:

{
  "editor.defaultFormatter": "esbenp.prettier-vscode",
  "editor.formatOnSave": true
}

3. Additionally, install the Prettier - Code formatter VSCode extension.

The next PR will set up GitHub Actions for Prettier.

[JGreenlee]

I love that Prettier instantly cleans up the file, but there are some cases where I think the code is actually less readable after being Prettified. I want to see if there's anything we can do to make the reformat less aggressive wrt the line breaks we already have in the code.

I'm glad you included a JS example, but I'd also love to see an example of Prettier applied to one of our TS files and one of our TSX files

[jiji14]

I love that Prettier instantly cleans up the file, but there are some cases where I think the code is actually less readable after being Prettified. I want to see if there's anything we can do to make the reformat less aggressive wrt the line breaks we already have in the code.

I'm glad you included a JS example, but I'd also love to see an example of Prettier applied to one of our TS files and one of our TSX files

I've added both TS and TSX files! While exploring customization options for line breaking, I found that Prettier offers limited flexibility due to its philosophy. Prettier has intentionally limited options due to its history.

For more information, you can refer to these resources:
Prettier's Option Philosophy
Stack Overflow Discussion

I'll continue searching for workarounds, and if you have any suggestions, please let me know!

[JGreenlee]

Yes, that StackOverflow thread demonstrates my concerns pretty clearly. Personally, I found the code clearer before reformatting. I could probably get used to it though.

What opinions do others have about the readability of the examples before and after being formatted with Prettier?
If there's no way to change this behavior, are we ok with the result?

[jiji14]

@shankari @JGreenlee @Abby-Wheelis @the-bay-kay @sebastianbarry @niccolopaganini

Other Prettier alternatives, such as JsFmt and StandardJS, also have difficulties in customizing formatting. This led me to consider ESLint, which includes formatting rules like semicolons and indent. While it doesn't provide an option for print width, it doesn't automatically break lines.

Here are the ESLint rules I applied :

    "rules": {
        "arrow-parens": [ "error", "always" ],
        "quotes": [ "error", "single" ],
        "semi": [ "error", "always" ],
        "eol-last": [ "error", "always" ],
        "indent": [ "error", 2 ],
        "no-multi-spaces": 2 // 2 means error
    },

Here are examples of the input-matcher.js code using ESLint.

'use strict';

import angular from 'angular';

angular.module('emission.survey.inputmatcher', ['emission.plugin.logger'])
  .factory('InputMatcher', function(Logger){
    var im = {};

    const EPOCH_MAXIMUM = 2**31 - 1;
    const fmtTs = function(ts_in_secs, tz) {
      return moment(ts_in_secs * 1000).tz(tz).format();
    };

    var printUserInput = function(ui) {
      return fmtTs(ui.data.start_ts, ui.metadata.time_zone) + '('+ui.data.start_ts + ') -> '+
           fmtTs(ui.data.end_ts, ui.metadata.time_zone) + '('+ui.data.end_ts + ')'+
           ' ' + ui.data.label + ' logged at '+ ui.metadata.write_ts;
    };

    im.validUserInputForDraftTrip = function(trip, userInput, logsEnabled) {
      if (logsEnabled) {
        Logger.log(`Draft trip:
            comparing user = ${fmtTs(userInput.data.start_ts, userInput.metadata.time_zone)}
            -> ${fmtTs(userInput.data.end_ts, userInput.metadata.time_zone)}
            trip = ${fmtTs(trip.start_ts, userInput.metadata.time_zone)}
            -> ${fmtTs(trip.end_ts, userInput.metadata.time_zone)}
            checks are (${userInput.data.start_ts >= trip.start_ts}
            && ${userInput.data.start_ts < trip.end_ts}
            || ${-(userInput.data.start_ts - trip.start_ts) <= 15 * 60})
            && ${userInput.data.end_ts <= trip.end_ts}
        `);
      }
      return (userInput.data.start_ts >= trip.start_ts
        && userInput.data.start_ts < trip.end_ts
        || -(userInput.data.start_ts - trip.start_ts) <= 15 * 60)
        && userInput.data.end_ts <= trip.end_ts;
    };

    im.validUserInputForTimelineEntry = function(tlEntry, userInput, logsEnabled) {
      if (!tlEntry.origin_key) return false;
      if (tlEntry.origin_key.includes('UNPROCESSED') == true)
        return im.validUserInputForDraftTrip(tlEntry, userInput, logsEnabled);

      /* Place-level inputs always have a key starting with 'manual/place', and
        trip-level inputs never have a key starting with 'manual/place'
       So if these don't match, we can immediately return false */
      const entryIsPlace = tlEntry.origin_key == 'analysis/confirmed_place';
      const isPlaceInput = (userInput.key || userInput.metadata.key).startsWith('manual/place');
      if (entryIsPlace != isPlaceInput)
        return false;

      let entryStart = tlEntry.start_ts || tlEntry.enter_ts;
      let entryEnd = tlEntry.end_ts || tlEntry.exit_ts;
      if (!entryStart && entryEnd) {
      // if a place has no enter time, this is the first start_place of the first composite trip object
      // so we will set the start time to the start of the day of the end time for the purpose of comparison
        entryStart = moment.unix(entryEnd).startOf('day').unix();
      }
      if (!entryEnd) {
        // if a place has no exit time, the user hasn't left there yet
        // so we will set the end time as high as possible for the purpose of comparison
        entryEnd = EPOCH_MAXIMUM;
      }
    
      if (logsEnabled) { 
        Logger.log(`Cleaned trip:
          comparing user = ${fmtTs(userInput.data.start_ts, userInput.metadata.time_zone)}
          -> ${fmtTs(userInput.data.end_ts, userInput.metadata.time_zone)}
          trip = ${fmtTs(entryStart, userInput.metadata.time_zone)}
          -> ${fmtTs(entryStart, userInput.metadata.time_zone)}
          start checks are ${userInput.data.start_ts >= entryStart}
          && ${userInput.data.start_ts < entryEnd}
          end checks are ${userInput.data.end_ts <= entryEnd}
          || ${userInput.data.end_ts - entryEnd <= 15 * 60})
        `);
      }

      /* For this input to match, it must begin after the start of the timelineEntry (inclusive)
        but before the end of the timelineEntry (exclusive) */
      const startChecks = userInput.data.start_ts >= entryStart &&
        userInput.data.start_ts < entryEnd;
      /* A matching user input must also finish before the end of the timelineEntry,
        or within 15 minutes. */
      var endChecks = (userInput.data.end_ts <= entryEnd ||
        (userInput.data.end_ts - entryEnd) <= 15 * 60);
      if (startChecks && !endChecks) {
        const nextEntryObj = tlEntry.getNextEntry();
        if (nextEntryObj) {
          const nextEntryEnd = nextEntryObj.end_ts || nextEntryObj.exit_ts;
          if (!nextEntryEnd) { // the last place will not have an exit_ts
            endChecks = true; // so we will just skip the end check
          } else {
            endChecks = userInput.data.end_ts <= nextEntryEnd;
            Logger.log('Second level of end checks when the next trip is defined('+userInput.data.end_ts+' <= '+ nextEntryEnd+') = '+endChecks);
          }
        } else {
          // next trip is not defined, last trip
          endChecks = (userInput.data.end_local_dt.day == userInput.data.start_local_dt.day);
          Logger.log('Second level of end checks for the last trip of the day');
          Logger.log('compare '+userInput.data.end_local_dt.day + ' with ' + userInput.data.start_local_dt.day + ' = ' + endChecks);
        }
        if (endChecks) {
          // If we have flipped the values, check to see that there
          // is sufficient overlap
          const overlapDuration = Math.min(userInput.data.end_ts, entryEnd) - Math.max(userInput.data.start_ts, entryStart);
          Logger.log('Flipped endCheck, overlap('+overlapDuration+
                ')/trip('+tlEntry.duration+') = '+ (overlapDuration / tlEntry.duration));
          endChecks = (overlapDuration/tlEntry.duration) > 0.5;
        }
      }
      return startChecks && endChecks;
    };

    // parallels get_not_deleted_candidates() in trip_queries.py
    const getNotDeletedCandidates = function(candidates) {
      console.log('getNotDeletedCandidates called with ' + candidates.length + ' candidates');
      // We want to retain all ACTIVE entries that have not been DELETED
      const allActiveList = candidates.filter((c) => !c.data.status || c.data.status == 'ACTIVE');
      const allDeletedIds = candidates.filter((c) => c.data.status && c.data.status == 'DELETED').map((c) => c.data['match_id']);
      const notDeletedActive = allActiveList.filter((c) => !allDeletedIds.includes(c.data['match_id']));
      console.log(`Found ${allActiveList.length} active entries,
                    ${allDeletedIds.length} deleted entries ->
                    ${notDeletedActive.length} non deleted active entries`);
      return notDeletedActive;
    };

    im.getUserInputForTrip = function(trip, nextTrip, userInputList) {
      const logsEnabled = userInputList.length < 20;

      if (userInputList === undefined) {
        Logger.log('In getUserInputForTrip, no user input, returning undefined');
        return undefined;
      }

      if (logsEnabled) {
        console.log('Input list = '+userInputList.map(printUserInput));
      }
      // undefined != true, so this covers the label view case as well
      var potentialCandidates = userInputList.filter((ui) => im.validUserInputForTimelineEntry(trip, ui, logsEnabled));
      if (potentialCandidates.length === 0) {
        if (logsEnabled) {
          Logger.log('In getUserInputForTripStartEnd, no potential candidates, returning []');
        }
        return undefined;
      }

      if (potentialCandidates.length === 1) {
        Logger.log('In getUserInputForTripStartEnd, one potential candidate, returning  '+ printUserInput(potentialCandidates[0]));
        return potentialCandidates[0];
      }

      Logger.log('potentialCandidates are '+potentialCandidates.map(printUserInput));
      var sortedPC = potentialCandidates.sort(function(pc1, pc2) {
        return pc2.metadata.write_ts - pc1.metadata.write_ts;
      });
      var mostRecentEntry = sortedPC[0];
      Logger.log('Returning mostRecentEntry '+printUserInput(mostRecentEntry));
      return mostRecentEntry;
    };

    // return array of matching additions for a trip or place
    im.getAdditionsForTimelineEntry = function(entry, additionsList) {
      const logsEnabled = additionsList.length < 20;

      if (additionsList === undefined) {
        Logger.log('In getAdditionsForTimelineEntry, no addition input, returning []');
        return [];
      }

      // get additions that have not been deleted
      // and filter out additions that do not start within the bounds of the timeline entry
      const notDeleted = getNotDeletedCandidates(additionsList);
      const matchingAdditions = notDeleted.filter((ui) => im.validUserInputForTimelineEntry(entry, ui, logsEnabled));

      if (logsEnabled) {
        console.log('Matching Addition list = '+matchingAdditions.map(printUserInput));
      }
      return matchingAdditions;
    };

    im.getUniqueEntries = function(combinedList) {
    // we should not get any non-ACTIVE entries here
    // since we have run filtering algorithms on both the phone and the server
      const allDeleted = combinedList.filter((c) => c.data.status && c.data.status == 'DELETED');
      if (allDeleted.length > 0) {
        Logger.displayError('Found '+allDeletedEntries.length
            +' non-ACTIVE addition entries while trying to dedup entries',
        allDeletedEntries);
      }
      const uniqueMap = new Map();
      combinedList.forEach((e) => {
        const existingVal = uniqueMap.get(e.data.match_id);
        // if the existing entry and the input entry don't match
        // and they are both active, we have an error
        // let's notify the user for now
        if (existingVal) {
          if ((existingVal.data.start_ts != e.data.start_ts) ||
                (existingVal.data.end_ts != e.data.end_ts) ||
                (existingVal.data.write_ts != e.data.write_ts)) {
            Logger.displayError('Found two ACTIVE entries with the same match ID but different timestamps '+existingVal.data.match_id,
              JSON.stringify(existingVal) + ' vs. '+ JSON.stringify(e));
          } else {
            console.log('Found two entries with match_id '+existingVal.data.match_id+' but they are identical');
          }
        } else {
          uniqueMap.set(e.data.match_id, e);
        }
      });
      return Array.from(uniqueMap.values());
    };

    return im;
  });

The reason I wanted to use `Prettier` for our project was because `Prettier` is widely used for standard `formatting`. ESLint, on the other hand, is used for maintaining code style rather than formatting. However, if Prettier makes our work difficult, I believe that `ESLint` would be a better choice for us.

P.S. For ts and tsx files, I'll need to set up the configuration!

[Abby-Wheelis]

The reason I wanted to use Prettier for our project was because Prettier is widely used for standard formatting. ESLint, on the other hand, is used for maintaining code style rather than formatting.

Looking at the diffs and the example above, I can see how this is the case. Prettier does a lot more to format the code than ESLint does, and I think that the changes do help with readability. While some of the changes it makes wrt linebreaks are a little visually jarring, I think they are better than doing nothing or leaving everything on one line. I like Prettier, under the understanding that I will get used to the more aggressive line breaking. It is a little annoying to look at, but I think I can understand why Prettier makes the following change, for example:

Logger.log('compare '+userInput.data.end_local_dt.day + ' with ' + userInput.data.start_local_dt.day + ' = ' + endChecks);

was broken to be

Logger.log(
            'compare ' +
              userInput.data.end_local_dt.day +
              ' with ' +
              userInput.data.start_local_dt.day +
              ' = ' +
              endChecks,
          );

Yes, it's a lot longer, but I think the idea of having each element on a separate line is growing on me, this does allow the different components to be seen at more of a glance than not line breaking would.

[jiji14]

Based on our discussion, everyone agreed that Prettier is the best option for our situation. I ran Prettier on all the files, except those specified in the .prettierignore setting.

The master branch was updated a few hours ago, causing significant merge conflicts. To resolve this issue, I reverted the code to its previous state, merged it into the current branch, and then ran Prettier again.

[shankari]

@jiji14 just to confirm, you don't have changes here other than the prettier formatting, correct?

[JGreenlee]

Glad we were able to implement this!

I have a few questions and requests:

• Make sure the .prettierignore has everything we need
• To run prettier, are we just doing npx prettier? Are any config options passed?
  • If any config options are passed, we should set up an npm command for it in our package.json "scripts"
• Did @shankari request that we run CI Prettier checks with Github Actions? Are we aiming to do that in this PR or on a different timeline?

[JGreenlee]

I think it will also be necessary to ignore the platforms directory, and maybe www/dist

Is there no way to target Prettier at only the www folder?

[JGreenlee]

/manual_lib and /json must be ignored as well

[jiji14]

Thanks for all of your advices!

I found the pattern only check www directory without dist, manual_lib, and json.
Can you please confirm this? 🙏

I ran prettier script locally with this setting and it worked well! I will push the code again if we don't need to add more files in .prettierignore!

[JGreenlee]

If the strategy is to ignore everything except www, you can remove node_modules and the artifacts since these are already ignored by the /* rule

[jiji14]

Oh, I missed that! Thanks :) I deleted the line!

[JGreenlee]

Can we remove the .editorconfig file now that we have Prettier?

[JGreenlee]

If we enforce Prettier here, I feel we must also enforce it in https://github.com/e-mission/e-mission-translate

[jiji14]

I agree with you! If @shankari and @Abby-Wheelis also agree, I will take care of e-mission-translate after finishing the setup for e-mission-translate.

[jiji14]

Glad we were able to implement this!

I have a few questions and requests:

• Make sure the .prettierignore has everything we need

• To run prettier, are we just doing npx prettier? Are any config options passed?

  • If any config options are passed, we should set up an npm command for it in our package.json "scripts"

• Did @shankari request that we run CI Prettier checks with Github Actions? Are we aiming to do that in this PR or on a different timeline?

1. I updated the .prettierignore file based on your advice.
2. We don't need to set up Prettier npm scripts for now because Prettier will automatically use the configuration from .prettierrc and the ignore rules specified in .prettierignore when we run the Prettier command (using npx prettier).
3. Yes, I will submit a separate PR for GitHub Actions after this one.

Please let me know if I missed anything else! If it looks good to you, I will re-run the script and push the final codes 😄

[JGreenlee]

@jiji14 I just cloned your branch, ran prettier myself, and looked at the result. It looks good to me!

However, to keep the commit history tidy, I suggest doing it on a new PR. I think it's preferable for every file will have just 1 commit for the reformat, instead of having the 'revert' commits show up (as you can see here, for example: https://github.com/jiji14/e-mission-phone/commits/prettier-setup/www/js/App.tsx)

[jiji14]

@jiji14 I just cloned your branch, ran prettier myself, and looked at the result. It looks good to me!

However, to keep the commit history tidy, I suggest doing it on a new PR. I think it's preferable for every file will have just 1 commit for the reformat, instead of having the 'revert' commits show up (as you can see here, for example: https://github.com/jiji14/e-mission-phone/commits/prettier-setup/www/js/App.tsx)
@JGreenlee
Apologize for the messy commit histories. 😿
To clarify, is this what you are looking for?

1. Close this PR.
2. Open a new PR, tagging this one to help us track what we've been discussing.
3. Create one commit for the formatting.

[JGreenlee]

@jiji14 Yes exactly 😊