-
Notifications
You must be signed in to change notification settings - Fork 595
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.
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
Proposal: Move csv/yaml/toml/json/jsonc/front_matter to top level #3123
Comments
I wasn't really following the discussion in many of the related issues. Three quick notes:
|
The move towards single-export files, which @timreichen aimed to do in his PRs as part of #2936.
Directory structure - perhaps having the various formats under the one directory was once okay, but now the implementations seemed to have outgrown
I don't think we should. Even though they're both JSON-related, they don't have much overlap. |
I was referring to #2660 in regards to this. We should probably make a decision on this as we are moving the files. |
I agree with @lino-levan as it seems like a dotland issue. Take a look at dotland. Say for example I'm looking for a JSON encoder. It's not particularly easy for me to look at the std library and see where it is. Searching isn't particularly easy either. Really the best way for me to find it is by searching through the GitHub source code. Conversely, look at Go stl. While their directory search doesn't seem to work for me, but at least I can "expand all" and search for it. |
I updated the 2nd paragraph of the issue description to clarify the purpose. Another important motivation is to resolve the current unreasonable structure of The discoverability in the web site can be solved by dotland repo, but there's also discoverability issue from IDE's import path completion. If we move these to the top level, they appear immediately after typing |
That's very interesting. I just realized that this is not a concern in Node or Golang. In Node, Intellisense knows what to do because of In Golang, it looks like autocomplete within the import statements is not a feature they had to begin with... Intellisense kicks in when writing code within the body, and offers up whatever is in the I wonder if |
I understand the concern here but I feel like this doesn't really affect a large group of people. If you've never touched deno before, you wouldn't know the structure of std and thus would not rely on ide autocomplete and go on the web to find what you are trying to do. If you are somewhat experienced in deno, you would know the location of the encoding (or would be able to figure it out pretty easily). To clarify: I'm not necessarily against moving the proposed modules to the top level, but I think:
|
I think it is a very big gain, because we will move towards a consistent file structure in std (no separate dirs and files for the same module as it is now).
I think the main reason for this change is consistency in the std file structure. That should be a good enough reason imo. |
Ideologically, I agree. We should have more than just ideological arguments for why we want to do things however (in my opinion). Is this a practical change? Is this issue a symptom of something else (outside of the file structure)? I think these are the questions that need discussion. |
The reasons for the change are to improve the file structure. A way to look at it is, being consistent with the rest of the repo, we would have CSV in a top-level folder. What valid reason would we have to make an exception and make it a submodule under Should the |
I personally don't think |
Practical aspect of this proposal (i.e. better discoverability) is just supporting points in my view. We usually don't do breaking changes just to make such slight discoverability improvements. The root motivation of this proposal is the resolution of the inconsistency in file/module structures. |
Sounds like fun. I'll do it. |
CC @MierenManz for any other opinions, as you're the original implementer of |
The way it's done in x/varint is slower for both u32 and u64 but for u32 it is possible to have it as fast as wasm. Just that u64 will be slow due to bigints not being that fast. I have a JavaScript version somewhere for u32 decoding and encoding which should be near wasm performance. For u64 it's just not possible to get the same performance I think |
Are there any other submodules that these changes could apply to? |
I think all are done now. Thanks Asher for these works! |
I just found this issue ... I really don't think that it makes sense to abandon structure just to cater to the poor search implementation of tooling (the deno.land website and the language server) when these shortcomings could very well be addressed without resorting to having all modules at the top-level. Please join #3462 for a discussion about that. |
These 6 modules in
std/encoding
are relatively complex and look worth top level modules.Also there are claims from contributors that the current structures of these modules are unreasonable/inconsistent (#2538). For example, csv related APIs are exported from
encoding/csv.ts
andencoding/csv/stream.ts
, and because of this structureencoding/csv
isn't in the correct form of module. (There's another proposal to moveencoding/csv.ts
toencoding/csv/mod.ts
, but by doing this these APIs will become less discoverable)I propose we should move these like the below:
encoding/csv.ts
->csv/mod.ts
encoding/yaml.ts
->yaml/mod.ts
encoding/toml.ts
->toml/mod.ts
encoding/json/*.ts
->json/mod.ts
encoding/jsonc.ts
->jsonc/mod.ts
encoding/front_matter.ts
->front_matter/mod.ts
related #2538
The text was updated successfully, but these errors were encountered: