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

Packages submodule, git files, and docs improvements #279

Merged
merged 7 commits into from
Jan 31, 2020
Merged

Packages submodule, git files, and docs improvements #279

merged 7 commits into from
Jan 31, 2020

Conversation

jrappen
Copy link
Contributor

@jrappen jrappen commented Jan 28, 2020

Update changelog:

  • added changelog info from releases to changelog file

Changes to Packages submodule target:

Copy link
Owner

@trishume trishume left a comment

Choose a reason for hiding this comment

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

Thanks! I like these changes, except for the CSS colors.

The CSS colors are clearly generated but the generation code isn't included and it's just pasted into a file that has other non-generated code. Also if the CSS colors are going to be used they'll need to be in some kind of map indexed by strings and not just Rust constants. I'd prefer you just remove the CSS color stuff from this PR, and if you plan on implementing sublime-color-scheme support you can include it there and ideally in a separate file containing only generated code and checking in the script that generated that file. Or use a different approach like the default syntax packs use where you load a binary dump of a string to color map that's generated by a program under examples.

Otherwise I like these changes, especially the much improved changelog and the new .gitattributes. If you remove the CSS color stuff I'm happy to merge this.

@trishume
Copy link
Owner

Oh also I think in order to get the Packages actually updated you have to do some check out step in the submodule. I forget exactly how submodules work but I'm pretty sure this PR doesn't actually update the packages, just makes future updates target the correct branch. Also when really updating the packages you need to rebuild the checked in binary dumps.

Not updating the packages in this PR is fine though. I'm pretty sure based on the other issues that updating the packages might not work.

reverting changes from 3015827 based on comment in #279
@jrappen
Copy link
Contributor Author

jrappen commented Jan 30, 2020

Can't test the build on this machine. Might try later today.

Pushed the requested changes regarding css names for now.

@jrappen jrappen changed the title Few minor improvements Packages submodule, git files, and docs improvements Jan 30, 2020
@trishume trishume merged commit 17b3a95 into trishume:master Jan 31, 2020
@jrappen jrappen mentioned this pull request Jan 5, 2021
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.

2 participants