This repository has been archived by the owner on Oct 4, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Metrics events - add utils to support mixpanel integration and persisting user settings in shell. #323
Metrics events - add utils to support mixpanel integration and persisting user settings in shell. #323
Changes from 14 commits
e2bc3c9
d986761
b229756
c799108
fd64c97
96ddf5a
1025c64
fc93a27
83b5c51
62b8288
e306f5c
eec7065
1e0a592
e6684af
1f79bbd
f99491d
2a80c73
9ae4d96
4df2336
80ff9d2
ad77f3c
ab9a4b7
cc93a4e
80caf92
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's already addressed in the part 2 of this effort which actually does events for all the things. can we let this be to avoid unnecessary churn?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be .near or .near-shell or .neardev?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also let's make this a constant? same for settings.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ilblackdragon I recommended Jane put it here as it fits with the structure I laid out in the NEP.
https://github.com/nearprotocol/NEPs/blob/974887126800be47a58e968fac5b997db7d50f7d/text/0000-near-shell.md#settings-config-and-key-management
(the same structure also appears in the home directory)
Based on the comment in the shell NEP, I understood that we were not going to put anything in
~/.near
as that's reserved for validator data. We can modify the NEP to be.neardev
if we want instead of.near-config
. Currentlyneardev
isn't a great name and holds private keys, which the NEP calls to change to a more informative directory.near-credentials
that folks aren't likely to accidentally revision. Anyway, that's the thought process. Again, we can modify the NEP.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved string literals to const. @mikedotexe thanks for the context for dir structure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need configs inside project directory?
I really think we should not store any credentials inside project directory - because that's the easiest way to lose them. E.g. #198
I also think settings should live in common directory for all projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ilblackdragon we don't need credentials, but per-project config is necessary. At the very least, it's important for making sure that people have updated deps e.g. shell itself. I agree that creds and global config should be in a common dir for everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@potatodepaulo read the NEP and left comments there - near/NEPs#31 (review)
High level, I don't think there is need in local configuration folder, especially one that starts with
.
as those directories should be gitignored.package.json
can contain settings that should be "clonnable".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we limit the discussion to user defined settings here in order to get the PR in for RL1?
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.