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.

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

Spilt Tagged<T> into a utility crate #3849

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Spilt Tagged<T> into a utility crate #3849

wants to merge 1 commit into from

Conversation

HalidOdat
Copy link
Member

@HalidOdat HalidOdat commented May 12, 2024

Depends on #3837

This makes Tagged<T> be usable in boa_string and boa_engine.

@HalidOdat HalidOdat added the Internal Category for changelog label May 12, 2024
@HalidOdat HalidOdat added this to the v0.18.1 milestone May 12, 2024
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 50,731 50,731 0
Passed 42,973 42,973 0
Ignored 1,395 1,395 0
Failed 6,363 6,363 0
Panics 18 18 0
Conformance 84.71% 84.71% 0.00%

@nekevss
Copy link
Member

nekevss commented May 13, 2024

General discussion on utility crates: how do we want to go about handling utilities in general that's going to be best for maintenance in the long term?

Should we have one general boa_utils crate or a utils directory with individual crates hosted inside it (I'm thinking of icu4x's structure as an example). We could also extract SmallMap into the utils as well. Both Tagged and SmallMap could then be standalone crates unto themselves.

There's some benefits to taking the utils directory approach.

  1. The code is more modular.
  2. Offers the libraries for generic use case for the Rust ecosystem as a whole.
  3. Could provide some soft "marketing" for boa, so to speak.

There could be a pretty big negative around the maintenance complexity utility crates may cause and managing separate releases may actually end being a large headache. 😕

Any thoughts?

@HalidOdat
Copy link
Member Author

Hmmm... Tagged<T> and SmallMap are very small (single file) so don't think it's going to need that much maintenance. Just a thought: we could put them in separate repos (like we do with ryu-js, and temporal_rs) may make it easier to separately publish the crates.

What does the rest of the team say? @jedel1043 @raskad @Razican

@Razican
Copy link
Member

Razican commented May 13, 2024

From my side, in general, if the crate / code makes sense outside Boa, then we should make it separate, so that others can benefit from it. If not, I would keep it in.

For this case, it seems these types could be used outside of Boa, and they have no dependency on the engine, so I would make them separate if it's not too much maintenance burden (and we don't forget to maintain them 😅)

@HalidOdat
Copy link
Member Author

I'm not sure what we should name the crate because tagged, tagged_ptr, tagged_pointer are all taken... Any ideas?

@jedel1043
Copy link
Member

I think separating small utility crates to separate repos is arguably worse for us. I would be in favour of having either a boa_utils or an utils directory here, but having to maintain a whole repo for only a single Tagged data structure sounds counter-productive.

@jedel1043
Copy link
Member

From my side, in general, if the crate / code makes sense outside Boa, then we should make it separate, so that others can benefit from it. If not, I would keep it in.

We can do that and also maintain it as a separate crate on our main repo. That's what the ICU4X folks do with writeable, yoke, etc. and it's not too bad. I think it only makes sense to extract separate repos if there's a big incentive to do so.

@nekevss
Copy link
Member

nekevss commented May 28, 2024

As mentioned above, I'd be for the utils directory with any small utility crates in them.

I'm not sure what we should name the crate because tagged, tagged_ptr, tagged_pointer are all taken... Any ideas?

tagged_rs maybe? tag_ptr could work although not super accurate 😕.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Category for changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants