ARMmbed / mbed-os-5-docs

Full documentation for Mbed OS 5 and 6
http://os.mbed.com/docs
98 stars 170 forks source link

Using `diff` for long modifications #1182

Closed ladislas closed 4 years ago

ladislas commented 4 years ago

Docs issue template

While reading the Custom Target Porting documentation, I found myself wishing to have long configuration changes displayed as diff instead of the final result. This could also be applied to short changes.

Page link

https://github.com/ARMmbed/mbed-os-5-docs/blob/development/docs/porting/custom-target-porting.md#customizing

Expected to find...

For long changes: (changes are not real, it just for the example)

{
  "IMAGINARYBOARD": {
-     "components_add": ["QSPIF"],
+     "components_add": ["QSPIF", "FLASHIAP"],
      "inherits": ["FAMILY_STM32"],
      "core": "Cortex-M4F",
      "extra_labels_add": ["STM32L4", "STM32L475xG", "STM32L475VG"],
      "config": {
          "clock_source": {
              "help": "Mask value : USE_PLL_HSE_EXTC (need HW patch) | USE_PLL_HSE_XTAL (need HW patch) | USE_PLL_HSI | USE_PLL_MSI",
              "value": "USE_PLL_MSI",
              "macro_name": "CLOCK_SOURCE"
          },
          "lpticker_lptim": {
              "help": "This target supports LPTIM. Set value 1 to use LPTIM for LPTICKER, or 0 to use RTC wakeup timer",
              "value": 1
          }
      },
      "overrides": { "lpticker_delay_ticks": 4 },
+     "detect_code": ["1234"],
      "macros_add": [
          "MBED_TICKLESS",
          "MBED_SPLIT_HEAP"
      ],
      "device_has_add": [
          "CRC",
          "TRNG",
          "FLASH",
          "QSPI",
          "MPU"
      ],
      "device_has_remove": [         
          "ANALOGIN",
          "I2CSLAVE",
          "I2C_ASYNCH"
      ],                     
      "release_versions": ["2", "5"],
      "device_name": "STM32L475VG",
-     "bootloader_supported": false
+     "bootloader_supported": true
  }
}

For example on small changes:

- I2C_SCL     = D15,
+ I2C_SCL     = PC_0,
- I2C_SDA     = D14,
+ I2C_SDA     = PC_1,

or

- I2C_SCL     = D15,
- I2C_SDA     = D14,
+ I2C_SCL     = PC_0,
+ I2C_SDA     = PC_1,

or

- I2C_SCL     = D15,
- I2C_SDA     = D14,

+ I2C_SCL     = PC_0,
+ I2C_SDA     = PC_1,

Found

I2C_SCL     = D15,
I2C_SDA     = D14,

to

I2C_SCL     = PC_0,
I2C_SDA     = PC_1,
ciarmcom commented 4 years ago

Internal Jira reference: https://jira.arm.com/browse/IOTDOCS-1334

AnotherButler commented 4 years ago

@ladislas Thank you for raising this issue. Our rendered website does not currently work with the diff feature. However, we can add code comments to these snippets to show what has changed.

ladislas commented 4 years ago

Comments sound good as well be could we have both? Maybe I'm the only one but I prefer reading the docs on Github where diff works ;)

AnotherButler commented 4 years ago

After looking into it, I don't think we can add comments. I don't think a .json allows any type of code comments. Do you think moving the changes above the code block would help?

ladislas commented 4 years ago

I don't think a .json allows any type of code comments

You're right, but is the json going to be used for real or just for documentation? If it's just for documentation, adding a comment as bellow is not a big deal IMHO.


{
  "json": [
    "rigid",
    "better for data interchange" # change here
  ],
  "yaml": [
    "slim and flexible", # change also here
    "better for configuration"
  ]
}

At least they stand out 😂

Another option is to keep the diff format but without highlighting. The - shows what you must remove and the + what you need to add.

- I2C_SCL     = D15,
- I2C_SDA     = D14,

+ I2C_SCL     = PC_0,
+ I2C_SDA     = PC_1,
AnotherButler commented 4 years ago

Changes need to be made to custom_targets.json to use Mbed OS on a custom board, so I don't think adding comments will work.

AnotherButler commented 4 years ago

I've created a PR that moves the changes above the snippet to make the changes more obvious: https://github.com/ARMmbed/mbed-os-5-docs/pull/1223

AnotherButler commented 4 years ago

Thanks again for raising this issue 👍 I'm closing it now that PR #1223 has merged.