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.

Sign up for GitHub

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

Jump to bottom

Quick add #1042

Merged
merged 3 commits into from
Feb 20, 2021
Merged

Quick add #1042

merged 3 commits into from
Feb 20, 2021

Conversation

shresthabijay
Copy link
Contributor

fixes #749

@shresthabijay
Copy link
Contributor Author

@raineorshine I have implemented the feature. I have not written any tests yet. Please review it. Thanks!

@raineorshine
Copy link
Contributor

raineorshine commented Feb 18, 2021 •
edited
Loading

I tested the functionality and it's exactly what I wanted! Thanks!

There are some bugs/edge cases I didn't anticipate. Would you prefer dealing with them in this PR or separate issues?

raineorshine
raineorshine requested changes Feb 18, 2021
View reviewed changes
Copy link
Contributor

@raineorshine raineorshine left a comment

Choose a reason for hiding this comment

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

Before I review, I'm sorry to have to ask for this, but can you rebase and do the renames in a separate commit? I know you'll have to rename them back to the originals, then rename them in a separate commit, but that will still be faster than me manually reviewing 139 files 😳.

@shresthabijay
Copy link
Contributor Author

Before I review, I'm sorry to have to ask for this, but can you rebase and do the renames in a separate commit? I know you'll have to rename them back to the originals, then rename them in a separate commit, but that will still be faster than me manually reviewing 139 files .

I still have to to change isRoot to something like isStartingContext and convert rootedParentOf to a selector. This will still affect many files. How about I separate and list the files that needs to reviewed from the files that only had rename changes ? Will it help ?

@raineorshine
Copy link
Contributor

I still have to to change isRoot to something like isStartingContext and convert rootedParentOf to a selector. This will still affect many files.

Sure. This could be done first, so it's separated from the other changes, right?

How about I separate and list the files that needs to reviewed from the files that only had rename changes ? Will it help ?

Yes, that would work. Seems like that involves the process suggested of creating a separate commit for the renames? Or are you thinking of doing it manually?

@shresthabijay
Copy link
Contributor Author

Yes, that would work. Seems like that involves the process suggested of creating a separate commit for the renames? Or are you thinking of doing it manually?

I think I will go with the suggested process.

@shresthabijay shresthabijay force-pushed the quick-add branch 3 times, most recently from ffc481d to 09eaf91 Compare February 19, 2021 11:08
@shresthabijay
Copy link
Contributor Author

shresthabijay commented Feb 19, 2021 •
edited
Loading

@raineorshine I renamed everything back and squashed everything to a single commit. The number of files changed is down to 59. I also then renamed everything for consistent semantics as we discussed.

You can just review the diff of the first commit. I hope this helps. First renamed commit!

@raineorshine
Copy link
Contributor

@shresthabijay

There are some bugs/edge cases I didn't anticipate. Would you prefer dealing with them in this PR or separate issues?

raineorshine
raineorshine requested changes Feb 19, 2021
View reviewed changes
Copy link
Contributor

@raineorshine raineorshine left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good! Thanks for dealing with the weird edge cases for this (the whole task is kind of a weird edge case!)

The change of rootedParentOf from a util to a selector should have been in its own commit. That would have made reviewing easier for me.

One subtle difference that I would have made, which I think is purely a judgment call, is to hardcode the values of TRANSIENT_ABSOLUTE_CHILD_PATH and ROOT_PATH_MAP rather than define constants. They're intuitive/motivated rather than arbitrary like ROOT_TOKEN or ABSOLUTE_TOKEN so they would make sense in context, they are trivially small, only used in two places, and hardcoding them helps minimize the surface area of the internal API. The downside is that if the transient path implementation were to change, the constant would make that a bit easier.

src/components/Content.tsx Outdated Show resolved Hide resolved
src/components/Content.tsx Outdated Show resolved Hide resolved
src/components/Content.tsx Outdated Show resolved Hide resolved
src/selectors/getRoot.ts Outdated Show resolved Hide resolved
src/selectors/rootedParentOf.ts Outdated Show resolved Hide resolved
src/components/Subthoughts.tsx Outdated Show resolved Hide resolved
src/selectors/getChildren.ts Outdated Show resolved Hide resolved
src/selectors/getChildren.ts Show resolved Hide resolved
src/util/isAbsolute.ts Outdated Show resolved Hide resolved
src/util/isHome.ts Outdated Show resolved Hide resolved
@shresthabijay
Copy link
Contributor Author

@raineorshine I have made the changes. The tests are passing in my system. But the tests are failing in the github. It seems to be the problem with latest dev.

@raineorshine
Copy link
Contributor

The tests are passing in my system. But the tests are failing in the github. It seems to be the problem with latest dev.

This happened when I added the dependency https://github.com/magic-akari/page-lifecycle#feat/add-types using npm v7. There is a bug in npm v7 that changes https:// to git+ssh:// in the package-lock.json, which then fails the CI install. It's being tracked here: npm/cli#2610.

We'll have to either stick to npm v6 or manually change git+ssh back to https in package-lock.json whenever installing dependencies.

@raineorshine raineorshine changed the base branch from dev to 29 February 20, 2021 17:33
@raineorshine raineorshine changed the base branch from 29 to dev February 20, 2021 17:33
raineorshine
raineorshine approved these changes Feb 20, 2021
View reviewed changes
src/components/Content.tsx
@@ -20,15 +20,20 @@ import { SimplePath } from '../types'
const tutorialLocal = localStorage['Settings/Tutorial'] === 'On'
const tutorialStepLocal = +(localStorage['Settings/Tutorial Step'] || 1)

const TransientChildPath = [{
Copy link
Contributor

Choose a reason for hiding this comment

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

camelCase

src/selectors/getRootPath.ts
import { SimplePath } from '../types'
import { State } from '../util/initialState'

const RootPathMap: Record<string, SimplePath> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

camelCase

@raineorshine raineorshine merged commit 3c891bf into cybersemics:dev Feb 20, 2021
@raineorshine
Copy link
Contributor

@shresthabijay Third time's the charm? :)

@shresthabijay

There are some bugs/edge cases I didn't anticipate. Would you prefer dealing with them in this PR or separate issues?

@shresthabijay
Copy link
Contributor Author

@shresthabijay Third time's the charm? :)

@shresthabijay

There are some bugs/edge cases I didn't anticipate. Would you prefer dealing with them in this PR or separate issues?

@raineorshine Sure :) Please describe the bugs/edge cases in new issues and assign them to me.

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

@raineorshine raineorshine raineorshine approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Quick Add
2 participants
@shresthabijay @raineorshine