Skip to content

[DRAFT] Update dependencies, add prettier/eslint for consistent style, add more type safety - #127

Draft
epsilonhalbe wants to merge 3 commits into
Richardsl:masterfrom
epsilonhalbe:fix_n_cleanu
Draft

[DRAFT] Update dependencies, add prettier/eslint for consistent style, add more type safety#127
epsilonhalbe wants to merge 3 commits into
Richardsl:masterfrom
epsilonhalbe:fix_n_cleanu

Conversation

@epsilonhalbe

Copy link
Copy Markdown

I noticed the dependencies were a bit out of date and the style was not very consistent. As well as a few as ... and ts-ignore.

I moved the ts file into a src folder and extracted default values and the domain model.

I ran the linter & formatter on the codebase, I tried to make it do as little changes as possible but the mix of commas/semicolons/quotes needed some changes to be unified.

the formatting & linting is cnotained in a single commit so it can be reverted more easily and review of unrelated content is easier.

Comment thread .editorconfig
charset = utf-8
end_of_line = lf
insert_final_newline = true
indent_style = space

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got warnings of mixed tabs and spaces => changed this to spaces

Comment thread .eslintignore
@@ -1,2 +1,2 @@
npm node_modules

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was wrong, there is no build directory and npm node_modules doesn't make sense

Comment thread src/main.ts
import { Plugin } from 'obsidian'
import HeatmapCalendarSettingsTab from 'settings'

declare global {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding the global declare removes the need for ts-ignore when accessing the window object

Comment thread src/main.ts

const intensities = calEntries
.map((e: Entry) => e.intensity)
.filter((intensity): intensity is number => intensity !== undefined)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a neat trick - to type filters for undefined. intensities with this has type number[] inferred by the compiler

Comment thread src/main.ts
parent: heatmapCalendarGraphDiv,
})

const months = ['Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun', 'Jul', 'Aug', 'Sep', 'Oct', 'Nov', 'Dec']

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplifies the previous duplication

Comment thread src/main.ts
}

async onload(): Promise<void> {
const settings = await this.loadSettings()

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

settings is only used here - no need for a global variable.

Comment thread src/main.ts

getWeekdayShort(dayNumber: number): string {
return new Date(1970, 0, dayNumber + this.settings.weekStartDay + 4).toLocaleDateString('en-US', {
getWeekdayShort(weekStartDay: number, dayNumber: number): string {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add parameter to avoid accessing a global variable

Comment thread src/main.ts Outdated
weekStartDay: 1,
}
export default class HeatmapCalendar extends Plugin {
settings: CalendarSettings

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing this since it is not needed, and this way we can enable strictPropertyInitialization in tsconfig

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant